-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Standardize list pattern lowering on Index
constructor.
#58055
Conversation
Fixes dotnet#57825. Relevant spec adjustment dotnet/csharplang#5484.
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.
LGTM (commit 3)
@jcouv, @dotnet/roslyn-compiler For the second review. |
@@ -432,6 +432,24 @@ private enum PatternIndexOffsetLoweringStrategy | |||
strategy = PatternIndexOffsetLoweringStrategy.UseAsIs; | |||
return VisitExpression(operand); | |||
} | |||
else if (unloweredExpr is BoundObjectCreationExpression { Constructor: MethodSymbol constructor, Arguments: { Length: 2 } arguments, ArgsToParamsOpt: { IsDefaultOrEmpty: true }, InitializerExpressionOpt: null } && | |||
(object)constructor == _compilation.GetWellKnownTypeMember(WellKnownMember.System_Index__ctor) && | |||
arguments[0] is { Type.SpecialType: SpecialType.System_Int32, ConstantValue.Value: int _ and >= 0 } index && |
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.
@@ -432,6 +432,24 @@ private enum PatternIndexOffsetLoweringStrategy | |||
strategy = PatternIndexOffsetLoweringStrategy.UseAsIs; | |||
return VisitExpression(operand); | |||
} | |||
else if (unloweredExpr is BoundObjectCreationExpression { Constructor: MethodSymbol constructor, Arguments: { Length: 2 } arguments, ArgsToParamsOpt: { IsDefaultOrEmpty: true }, InitializerExpressionOpt: null } && |
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.
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.
do we have a indexing test with an Index initialization with an initializer?
No, the type doesn't have any member that can be initialized, but I decided to have the check for completeness.
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.
LGTM Thanks! (iteration 3)
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
…rovements * upstream/main: (68 commits) Lazy load ISourceLinkService to reduce DLL loads (dotnet#58108) [main] Update dependencies from dotnet/source-build (dotnet#57707) [main] Update dependencies from dotnet/arcade (dotnet#57968) Factor nullability logic for placeholders (dotnet#58036) Standardize list pattern lowering on `Index` constructor. (dotnet#58055) Add scripts to verify if a branch is ready to review Merge pull request dotnet#58100 from dotnet/dev/jorobich/skip-test Fix some places we weren't correctly disposing of VisualStudioAnalyzers Fix analyzer references being removed and added in one batch Fix indenting Ensure we don't silently capture any exceptions Don't MEF import the implementation directly if the public type will do Change comment Add comment Use actual jump tables Remove unused function Revert Simplify code Compute kind on demand Reorder ...
Fixes #57825.
Relevant spec adjustment dotnet/csharplang#5484.
Relates to test plan: #51289