-
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
Use new record equality specification #43626
Conversation
Per design changes, record equality is now defined using all instance fields, instead of just record fields.
src/Compilers/CSharp/Test/Symbol/Symbols/AnonymousTypesSymbolTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/AnonymousTypesSymbolTests.cs
Outdated
Show resolved
Hide resolved
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 (other than the miss-add/move of anonymous type symbol tests).
ping @gafter for review |
src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEquals.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEquals.cs
Outdated
Show resolved
Hide resolved
: F.LogicalAnd(retExpression, nextEquals); | ||
} | ||
|
||
RoslynDebug.AssertNotNull(retExpression); |
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.
AssertNotNull [](start = 24, length = 13)
This assertion does not appear justified.
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.
Earlier fields
is asserted to have more than 1 element, there will always be at least one iteration of the foreach
.
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.
How is there always at least one iteration of the foreach? According to the spec a record can have an empty parameter list.
In reply to: 418268985 [](ancestors = 418268985)
Finished a quick scan of Iteration 2. |
TList fields, | ||
SyntheticBoundNodeFactory F) where TList : IReadOnlyList<FieldSymbol> | ||
{ | ||
Debug.Assert(fields.Count > 0); |
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.
fields.Count > 0 [](start = 25, length = 16)
How is this justified? Please add a PROTOYPE, issue, or bullet in the test plan.
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.
Looks good, except for a possible mismatch with the spec on how to handle a data class declared with an empty parameter list. Please track that somehow.
Per design changes, record equality is now defined using all instance
fields, instead of just record fields.
Spec changes at dotnet/csharplang#3396