Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Optimize single spread collection expression for
List<T>
#74769Optimize single spread collection expression for
List<T>
#74769Changes from 1 commit
b3570d7
a973cc8
ed9f0c6
bc71192
c7a3f51
dfa95e5
6895189
0a66f09
7e5b188
bd468b4
a6e8878
05ffe61
95274af
c53a218
ae1c2be
2c8ac16
4125d52
23d22b2
2234846
4c710c1
e817f62
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should we maybe use the actual use site info for thisand report it if we take the optimzal code path an the end? Unline the
ICollection<T>
check, the resulting conversion is kinda used in the final codegn, although implicitlyThere 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 guess the original code for the AddRange case was reporting the use-site diagnostic, but I think it's fine to drop it. The code we generate does not actually use this type in any way, it's just a hint for us to decide if AddRange/ToList is likely to be efficient.
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.
Let's use the code from
tryOptimizeSpreadElement
which checks forICollection<T>
, then checks the conversion toaddRangeMethod.Parameters[0].Type
rather than the conversion toIEnumerable<T>
directly. But perhaps we should useClassifyImplicitConversionFromType()
rather thanClassifyConversionFromType()
for the parameter conversion check. Also, the comment in theICollection<T>
check fromtryOptimizeSpreadElement
would be good to keep. Obviously, the references toAddRange
should be updated though.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.
Also, the
ICollection<T>
check intryOptimizeSpreadElement
usedspreadElement.EnumeratorInfo.CollectionType
andspreadElement.EnumeratorInfo.ElementType
. With this change, we are usingspreadElement.Expression.Type
and the element type from the target type. Is there a difference between those pairs of types?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 didn't implement logic in the binding layer, that fills in
EnumeratorInfoOpt
, but from the sanity point of view they should be the same. I preffer avoidingEnumeratorInfoOpt
since it can benull
. Or maybe we can assume, that at the point of lowering we always have it and drop additionalnull
checks?