-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add Enumerable.TryGetNonEnumeratedCount (Implements #27183) #48239
Add Enumerable.TryGetNonEnumeratedCount (Implements #27183) #48239
Conversation
Tagging subscribers to this area: @eiriktsarpalis Issue DetailsFix #27183.
|
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. |
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.
Other than the build breaks, LGTM.
@@ -190,7 +190,7 @@ public void Reserve(int count) | |||
public bool ReserveOrAdd(IEnumerable<T> items) | |||
{ | |||
int itemCount; | |||
if (EnumerableHelpers.TryGetCount(items, out itemCount)) | |||
if (System.Linq.Enumerable.TryGetNonEnumeratedCount(items, out itemCount)) |
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.
Nit: using System.Linq;
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 wanted to avoid bringing all the enumerable methods into scope since this is a System.Collections namespace.
/// The method is typically a constant-time operation, but ultimately this depends on the complexity | ||
/// characteristics of the underlying collection implementation. | ||
/// </remarks> | ||
public static bool TryGetNonEnumeratedCount<TSource>(this IEnumerable<TSource> source, out int 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.
This doesn't need to be generic? if it does, I think a non-generic overload would also make sense.
(roslyn is considering using this method as part of list pattern lowering, we probably don't want to skip it for IEnumerable
see https://github.com/dotnet/csharplang/blob/master/meetings/2021/LDM-2021-02-03.md).
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.
We could do that, however it becomes more difficult to check for generic interfaces, which would require some form of reflection.
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.
which would require some form of reflection.
That's definitely a no-go because the point of using this is performance. Perhaps those generic interfaces need a non-generic base with Count
prop. IMO TryGetNonEnumeratedCount
shouldn't care about TSource
.
Linker test build failures seem unrelated to the change. |
Fix #27183.