-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve ArraySegment debugging #90488
Conversation
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsFixes #88961
|
/// For <see cref="ArraySegment{Char}"/>, returns a new instance of string that represents the characters pointed to by the segment. | ||
/// Otherwise, returns a <see cref="string"/> with the name of the type and the number of elements. | ||
/// </summary> | ||
public override string ToString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ToString
is an observable behavior change. The common approach is using a private property called DebuggerDisplay
.
if (typeof(T) == typeof(char)) | ||
{ | ||
StringBuilder stringBuilder = new StringBuilder(_count); | ||
|
||
for (int i = 0; i < _count; i++) | ||
{ | ||
T element = this[i]; | ||
stringBuilder.Append(Unsafe.As<T, char>(ref element)); | ||
} | ||
|
||
return stringBuilder.ToString(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use a StringBuilder
, just call string constructor like ReadOnlySpan<T>.ToString
.
{ | ||
if (typeof(T) == typeof(char)) | ||
{ | ||
return new string(new ReadOnlySpan<char>(ref Unsafe.As<T, char>(ref _array![_offset]), _count)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return new string(new ReadOnlySpan<char>(ref Unsafe.As<T, char>(ref _array![_offset]), _count)); | |
return AsSpan().ToString(); |
private sealed class ArraySegmentDebugView<U> | ||
{ | ||
private readonly U[] _array; | ||
|
||
public ArraySegmentDebugView(ArraySegment<U> arraySegments) | ||
{ | ||
_array = arraySegments.ToArray(); | ||
} | ||
|
||
[DebuggerBrowsable(DebuggerBrowsableState.RootHidden)] | ||
public U[] Items => _array; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private sealed class ArraySegmentDebugView<U> | |
{ | |
private readonly U[] _array; | |
public ArraySegmentDebugView(ArraySegment<U> arraySegments) | |
{ | |
_array = arraySegments.ToArray(); | |
} | |
[DebuggerBrowsable(DebuggerBrowsableState.RootHidden)] | |
public U[] Items => _array; | |
} | |
private sealed class ArraySegmentDebugView(ArraySegment<T> array) | |
{ | |
[DebuggerBrowsable(DebuggerBrowsableState.RootHidden)] | |
public T[] Items => array.AsSpan().ToArray(); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we started to roll-in primary constructor in runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for simple uses like debugger type proxies.
private sealed class ArraySegmentDebugView<U>(ArraySegment<U> array) | ||
{ | ||
[DebuggerBrowsable(DebuggerBrowsableState.RootHidden)] | ||
public U[] Items => array.AsSpan().ToArray(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a nested type, this should not introduce another generic type parameter.
private sealed class ArraySegmentDebugView<U>(ArraySegment<U> array) | |
{ | |
[DebuggerBrowsable(DebuggerBrowsableState.RootHidden)] | |
public U[] Items => array.AsSpan().ToArray(); | |
} | |
private sealed class ArraySegmentDebugView(ArraySegment<T> array) | |
{ | |
[DebuggerBrowsable(DebuggerBrowsableState.RootHidden)] | |
public T[] Items => array.AsSpan().ToArray(); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fails on compilation with CS0416 error : '.System.ArraySegment.ArraySegmentDebugView': an attribute argument cannot use type parameters
See https://github.com/dotnet/runtime/runs/15879904568
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The attribute should be:
[DebuggerTypeProxy(typeof(ArraySegment<>.ArraySegmentDebugView))]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the help ^^
return this.AsSpan().ToString(); | ||
} | ||
|
||
return $"System.ArraySegment<{typeof(T).Name}>[{_count}]"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original issue wanted the elements to be displayed in such case I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand. What is the issue exactly with this implementation ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original issue wanted the elements to be displayed in such case I think.
We don't do that for other collections. I'm not sure why we'd do it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can someone explains to me what should be done here ? I used the same code as the one for Span
See Span debugging support for ideas of improvement.
runtime/src/libraries/System.Private.CoreLib/src/System/Span.cs
Lines 376 to 387 in ac02b66
/// <summary> | |
/// For <see cref="Span{Char}"/>, returns a new instance of string that represents the characters pointed to by the span. | |
/// Otherwise, returns a <see cref="string"/> with the name of the type and the number of elements. | |
/// </summary> | |
public override string ToString() | |
{ | |
if (typeof(T) == typeof(char)) | |
{ | |
return new string(new ReadOnlySpan<char>(ref Unsafe.As<T, char>(ref _reference), _length)); | |
} | |
return $"System.Span<{typeof(T).Name}>[{_length}]"; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For types with a Count
such as this, the debugger display usually displays Count = XXX
.
e.g.
runtime/src/libraries/System.ObjectModel/src/System/Collections/ObjectModel/KeyedCollection.cs
Line 12 in f9b07d4
[DebuggerDisplay("Count = {Count}")] |
I would lean towards doing that. However, I don't own this code. @stephentoub or someone else on the dotnet/runtime team, could you clarify what the debugger display should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedrobsaila thank you for your contribution!
I think that we can simplify the solution, PTAL at my suggestion. Thank you!
[DebuggerDisplay("{DebuggerToString(),nq}")] | ||
[DebuggerTypeProxy(typeof(ArraySegment<>.ArraySegmentDebugView))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that we should simply mimic what List<T>
does:
ArraySegment<T>
implements IList<T>
which extends ICollection<T>
so it should work OOTB without any other code changes being required
[DebuggerDisplay("{DebuggerToString(),nq}")] | |
[DebuggerTypeProxy(typeof(ArraySegment<>.ArraySegmentDebugView))] | |
[DebuggerTypeProxy(typeof(ICollectionDebugView<>))] | |
[DebuggerDisplay("Count = {Count}")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @pedrobsaila !
Fixes #88961