-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Use char overload in System.Xaml #3998
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
Use char overload in System.Xaml #3998
Conversation
| { | ||
| NameFixupToken token = value as NameFixupToken; | ||
| string names = String.Join(",", token.NeededNames.ToArray()); | ||
| string names = String.Join(',', token.NeededNames.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.
Curious, are these ToArray calls needed? It seems like NeededNames implements IEnumerable<String>. (Not that it probably matters on this exception oath)
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 would definetly be worth benchmarking but List may have optimization that makes it quick to return a copy of the internal array so I don't know if it is faster than enumerating with IEnumerable. My guess would be that ToArray is faster but uses more memory (It may vary with the number of item).
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.
List creates a new array and copies elements over, while enumeration uses in-place value-type enumerator. It's a one method call vs. method call every item during enumerating, but also don't forget that the string.Join implementation differs - the array uses unsafe, pre-allocated char buffer while the enumerable uses StringBuilder to avoid double enumeration, and another difference is that changing the collection while being enumerated is illegal and throws.
That said, it probably doesn't matter here as it's an exception path and the number of items in the list would typically be quite small.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Thank you for your contribution, @ThomasGoulet73. |
Replace all method call that uses a single char string while there is another method that accepts a char.
Should yield tiny performance benefit on hot paths.