Skip to content

Add more ref readonly tests and address comments#69183

Merged
jjonescz merged 8 commits intodotnet:mainfrom
jjonescz:RefReadonly-19-Tests
Jul 28, 2023
Merged

Add more ref readonly tests and address comments#69183
jjonescz merged 8 commits intodotnet:mainfrom
jjonescz:RefReadonly-19-Tests

Conversation

@jjonescz
Copy link
Copy Markdown
Member

@jjonescz jjonescz commented Jul 24, 2023 •
edited
Loading

Test plan: #68056

@ghost ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 24, 2023
@jjonescz jjonescz added the Feature - Ref Readonly Parameters `ref readonly` parameters label Jul 24, 2023
@jjonescz jjonescz requested review from AlekseyTs and jcouv July 24, 2023 13:50
@jjonescz jjonescz marked this pull request as ready for review July 24, 2023 13:50
@jjonescz jjonescz requested a review from a team as a code owner July 24, 2023 13:50
AlekseyTs
AlekseyTs reviewed Jul 24, 2023
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs Outdated
$"Rely on the {nameof(refKindCompareMode)} flag to set this to ensure all cases are handled.");
Debug.Assert((_refKindCompareMode & RefKindCompareMode.AllowRefReadonlyVsInMismatch) == 0 ||
(_refKindCompareMode & RefKindCompareMode.ConsiderDifferences) != 0,
$"Cannot set {nameof(RefKindCompareMode.AllowRefReadonlyVsInMismatch)} without {nameof(RefKindCompareMode.ConsiderDifferences)}.");
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Consider making the assert more general. I.e. either _refKindCompareMode == DoNotConsiderDifferences, or the ConsiderDifferences flag is set. #Closed

AlekseyTs
AlekseyTs reviewed Jul 24, 2023
View reviewed changes
Comment thread src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs Outdated
}

[Fact]
public void ExpressionTrees()
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ExpressionTrees

I think we also should test calling methods with ref readonly parameters inside an expression tree, With all possible call site modifiers, including none. Also, passing an rValue case. #Closed

AlekseyTs
AlekseyTs reviewed Jul 24, 2023
View reviewed changes
Comment thread src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs
}

[Fact]
public void Implementation_RefReadonly_In_Close()
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Implementation_RefReadonly_In_Close

What is the goal of the test? Should we test success scenario instead by making the methods in C public? #Closed

Copy link
Copy Markdown
Member Author

@jjonescz jjonescz Jul 24, 2023

Choose a reason for hiding this comment

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

No, the point is to test MemberSignatureComparer.CSharpCloseImplicitImplementationComparer

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Jul 24, 2023

Done with review pass (commit 5)

@jjonescz jjonescz requested a review from AlekseyTs July 25, 2023 09:56
AlekseyTs
AlekseyTs approved these changes Jul 25, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 8)

@jjonescz jjonescz changed the base branch from features/RefReadonly to main July 26, 2023 16:43
@jjonescz
Copy link
Copy Markdown
Member Author

jjonescz commented Jul 27, 2023

@jcouv for a second review, thanks

jcouv
jcouv reviewed Jul 27, 2023
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs

return true;

static bool areRefKindsCompatible(RefKindCompareMode refKindCompareMode, RefKind refKind1, RefKind refKind2)
Copy link
Copy Markdown
Member

@jcouv jcouv Jul 27, 2023

Choose a reason for hiding this comment

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

Thanks. This was much easier to follow :-)

jcouv
jcouv approved these changes Jul 27, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 8)

@jcouv jcouv self-assigned this Jul 27, 2023
@jcouv jcouv added this to the 17.8 milestone Jul 27, 2023
@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jul 27, 2023
@jjonescz jjonescz merged commit 218d39d into dotnet:main Jul 28, 2023
@jjonescz jjonescz deleted the RefReadonly-19-Tests branch July 28, 2023 07:28
@ghost ghost modified the milestones: 17.8, Next Jul 28, 2023
@allisonchou allisonchou mentioned this pull request Aug 2, 2023
Closed
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels

Area-Compilers Feature - Ref Readonly Parameters `ref readonly` parameters

Projects

None yet

Milestone

17.8 P2

Development

Successfully merging this pull request may close these issues.

4 participants

@jjonescz @AlekseyTs @jcouv @dibarbet