Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement auto-generated Equals for records #41401

Merged
merged 6 commits into from
Feb 28, 2020
Merged

Implement auto-generated Equals for records #41401

merged 6 commits into from
Feb 28, 2020

Conversation

agocke
Copy link
Member

@agocke agocke commented Feb 4, 2020 •
edited by jcouv
Loading

Generates a strongly-typed Equals method and an override of the
object.Equals method.

Relates to #40726 (test plan for records)

@agocke agocke force-pushed the key-equals branch 3 times, most recently from df3e969 to 43dd43c Compare February 6, 2020 18:53
@agocke
Implement auto-generated Equals for records
32b4b4e 
Generates a strongly-typed Equals method and an override of the
object.Equals method.
@agocke agocke marked this pull request as ready for review February 8, 2020 05:48
@agocke agocke requested a review from a team as a code owner February 8, 2020 05:48
@agocke agocke closed this Feb 8, 2020
@agocke agocke reopened this Feb 8, 2020
@gafter gafter self-requested a review February 10, 2020 22:27
@gafter gafter requested a review from 333fred February 10, 2020 22:28
@gafter
Copy link
Member

gafter commented Feb 10, 2020 •
edited
Loading

// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

Wrong license. #Resolved

Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedRecordPropertySymbol.cs:1 in 32b4b4e. [](commit_id = 32b4b4e, deletion_comment = False)

@gafter
Copy link
Member

gafter commented Feb 10, 2020 •
edited
Loading

// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

Wrong license. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:1 in 32b4b4e. [](commit_id = 32b4b4e, deletion_comment = False)

@gafter
Copy link
Member

gafter commented Feb 10, 2020

Done reviewing Iteration 1.

@agocke
Copy link
Member Author

agocke commented Feb 14, 2020

@gafter I think I've addressed your comments.

gafter
gafter approved these changes Feb 15, 2020
View reviewed changes
Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@agocke
Copy link
Member Author

agocke commented Feb 18, 2020

@333fred Can you take a look when you get a chance?

@agocke agocke closed this Feb 19, 2020
@agocke agocke reopened this Feb 19, 2020
@agocke agocke closed this Feb 20, 2020
@agocke agocke reopened this Feb 20, 2020
@agocke
Copy link
Member Author

agocke commented Feb 25, 2020

@dotnet/roslyn-compiler Please review

333fred
333fred reviewed Feb 25, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordObjEquals.cs
{
var F = new SyntheticBoundNodeFactory(this, ContainingType.GetNonNullSyntaxNode(), compilationState, diagnostics);

// return this.Equals(param as ContainingType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a quick reference equality check to avoid the possibly-unnecessary cast?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now I'm not concerned with the performance of the implementation. That's something I'll look at later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a prototype comment to track then?

@333fred
Copy link
Member

333fred commented Feb 25, 2020

Done review pass (commit 4)

@agocke
Copy link
Member Author

agocke commented Feb 27, 2020

@333fred Anything else?

333fred
333fred approved these changes Feb 27, 2020
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to see a PROTOTYPE around the equality perf part, but otherwise LGTM (commit 6)

@agocke
Copy link
Member Author

agocke commented Feb 28, 2020

@333fred It's no worse than what we're doing for anonymous types, so it's not exactly bad.

@agocke agocke merged commit 0f5e24f into dotnet:features/records Feb 28, 2020
@agocke agocke deleted the key-equals branch February 28, 2020 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@333fred 333fred 333fred approved these changes

@gafter gafter gafter approved these changes

Assignees
No one assigned
Labels
Area-Compilers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@agocke @gafter @333fred