-
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
Add Enumerable.Concat & Enumerable.Flatten methods #61230
Add Enumerable.Concat & Enumerable.Flatten methods #61230
Conversation
Removes the ExceptionArgument mechanism that seems to predate the nameof language feature.
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. |
Tagging subscribers to this area: @dotnet/area-system-linq Issue Details
Fixes #54220.
|
Why is it "unneeded"? |
ThrowHelper.ThrowArgumentNullException(nameof(sources)); | ||
} | ||
|
||
return FlattenIterator(sources); |
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.
Did you consider just doing:
return sources.SelectMany(e => e);
rather than implementing another iterator?
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 did, ultimately it's tradeoff between this and a closure allocation and a few virtual calls. I should point out that the simple SelectMany
implementation does inherit from Iterator<T>
, but ultimately I didn't find any of the speed optimizations offered in this particular case to be useful in practice.
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 did, ultimately it's tradeoff between this and a closure allocation and a few virtual calls
There's no closure allocation with sources.SelectMany(e => e)
. There is one delegate per T allocated for the process. And an extra delegate invocation per enumerable. From my perspective, I'd rather have less code to maintain and a smaller binary (and implicitly pick up any optimizations SelectMany adds in the future, e.g. if it were to special case T[]). Up to you, though.
I would guess it's a mechanism that predates |
It's not. It's reducing the size of the generated generic instantiations. It's possible it's no longer the right tradeoff, but that would benefit from numbers. |
If I understand this correctly, it's an optimization replacing an |
This reverts commit 231b537.
Note to reviewers: the feature is implemented in dfd7d00 and should be reviewed separately from 231b537 which contains infrastructural cleanups.
Fixes #54220.