Readonly members prototype cleanup #34468
Conversation
| // Equals(null); | ||
| Diagnostic(ErrorCode.WRN_ImplicitCopyInReadOnlyMember, "Equals").WithArguments("Equals", "this").WithLocation(14, 9)); | ||
|
|
||
| verifier.VerifyIL("S1.M", @" |
There was a problem hiding this comment.
VerifyIL [](start = 21, length = 8)
nit: consider adding a comment to point out the important thing you're trying to verify in the IL #Closed
There was a problem hiding this comment.
Will do. Will also split this test into two and migrate one more base method test over from ReadOnlyStructsTests. Should help with readability.
In reply to: 270201319 [](ancestors = 270201319)
There was a problem hiding this comment.
nit: I'm not sure the split between two test files helps (whenever a test moves, the review becomes more confusing).
In reply to: 270204398 [](ancestors = 270204398,270201319)
There was a problem hiding this comment.
Ah, do you think I should just leave it in its original file even though it's now verifying IL? I'm fine with that. #Closed
|
Test plan review has revealed a few more minor changes that should be done before the feature is merged, but I am going to do them in another PR for reviewability's sake. |
|
@dotnet/roslyn-compiler could I get a second review please? |
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenReadonlyStructTests.cs
Outdated
Show resolved
Hide resolved
Related to #32911
This PR has several minor fixes and adds coverage for some new scenarios.
ThisParameterSymbol.RefKindAPI to use RefKind.In when containingMethod.IsEffectivelyReadOnlyreadonly partialmethods: both signatures must containreadonly. Is this what we want?The last few items might need to be hashed out in review and the speclet should probably be updated to be clear about the expected behavior. (done)