System.Diagnostics.Activity: Implement Enumerate* API #67920
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
ghost
added
area-System.Diagnostics.Activity
new-api-needs-documentation
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-activity Issue DetailsFixes #67207 /cc @reyang @cijothomas @tarekgh @noahfalk
|
src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs
Show resolved
Hide resolved
|
Thanks @CodeBlanch for helping all the way with this issue. I'll wait @noahfalk feedback and then will merge it. |
|
@CodeBlanch one last thing I forgot to ask, could you please ensure the return values of the methods is working fine with |
|
@tarekgh Sure, np. Added |
|
|
||
| public struct Enumerator<T> | ||
| { | ||
| public readonly System.Diagnostics.Activity.Enumerator<T> GetEnumerator() { throw null; } |
There was a problem hiding this comment.
Is this just here to make the C# compiler happy when doing a foreach loop? It feels like we are abusing the Enumerable/Enumerator pattern a bit but as long as BCL review team was happy with it I am happy too : )
There was a problem hiding this comment.
Is this just here to make the C# compiler happy when doing a foreach loop?
Yes
It feels like we are abusing the Enumerable/Enumerator pattern a bit but as long as BCL review team was happy with it I am happy too : )
Actually this was recommended by the BCL design reviewers :-)
There was a problem hiding this comment.
Just FYI here are a couple other examples of this being done that came up during the review:
Brings up a question, should we add...
[ComponentModel.EditorBrowsable(ComponentModel.EditorBrowsableState.Never)]...like ChunkEnumerator is doing?
There was a problem hiding this comment.
Just FYI here are a couple other examples of this being done that came up during the review:
Actually, StringRuneEnumerator is implementing IEnumerable<>.
Brings up a question, should we add...
Good point. Please go ahead and apply the attribute as we did in StringBuilder
There was a problem hiding this comment.
Looks you need to update the ref file too.
There was a problem hiding this comment.
Updated. Sorry rookie move 😄
|
Are we expecting any perf impact here? Possible regression seen on arm64: #68624 |
|
@AndyAyersMS looking at the perf test code I cannot relate the changes in this PR to the operations we are doing in the test. All changes in this PR mostly for some properties enumeration which is not used in the test at all. |
ghost
locked as resolved and limited conversation to collaborators
4 participants
Fixes #67207
/cc @reyang @cijothomas @tarekgh @noahfalk