-
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
Avoid returning default from MethodSymbol.Parameters #54179
Conversation
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 4)
@dotnet/roslyn-compiler For the second review. |
@@ -106,7 +106,7 @@ public sealed override ImmutableArray<ParameterSymbol> Parameters | |||
{ | |||
if (_parameters.IsDefault) | |||
{ | |||
ImmutableInterlocked.InterlockedCompareExchange(ref _parameters, MakeParameters(), default(ImmutableArray<ParameterSymbol>)); | |||
ImmutableInterlocked.InterlockedInitialize(ref _parameters, MakeParameters()); |
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.
Can you help me understand how these two lines are different? I'm missing the distinction here.
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.
➡️ They are semantically identical. The simpler form allows a reader to understand the behavior without the mental burden of relating the third parameter of InterlockedCompareExchange
to the default state for the field.
Merged/squashed. Will take another look at https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1341051 with this new baseline. Thanks |
@jcouv Please do not squash merge pull requests submitted by me. I meticulously separated this pull request into four distinct changes with the intent they would be preserved as such. Thanks! |
@sharwell Sorry about that. |
@jcouv no problem glad it's merged ! |
Review commit-by-commit recommended.
This change ensures that all implementations of
MethodSymbol.Parameters
return a non-default array, excluding cases whereLazyMethodChecks
fails to initialize parameters. It also cleans up some initialization sites to use current practices.