-
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
Disallow TypeSpec after CLASS and VALUETYPE in signatures #8027
Conversation
@nguerrera @AlekseyTs @dotnet/roslyn-compiler |
|
||
File.WriteAllBytes( | ||
Args[0], | ||
BitConverter.ToString(File.ReadAllBytes(Args[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.
Nit against my own repro code here. I meant to use the file local instead of repeating Args[0] twice.
Some small comments, but LGTM. |
<Content Include="MetadataTests\Invalid\Signatures\munge.csx" /> | ||
<Content Include="MetadataTests\Invalid\Signatures\SignatureCycle2.il" /> | ||
<Content Include="MetadataTests\Invalid\Signatures\TypeSpecInWrongPlace.il" /> | ||
<EmbeddedResource Include="MetadataTests\Invalid\Signatures\SignatureCycle2.exe" /> |
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 we please stop checking in .exe files? Unless it is truly tiny.
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.
It is tiny (2KB). This is the current pattern we use in compilers, I don't see what's wrong with it. The file will never change.
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.
BTW, as soon as we'll have a metadata writer it will be much easier to write these tests without checking in binaries. But for now this is what we do.
LGTM |
@dotnet/roslyn-compiler Can I get one more review please? |
|
||
[WorkItem(7971, "https://github.com/dotnet/roslyn/issues/7971")] | ||
[Fact(Skip = "7971")] | ||
public void MemberSignature_CycleTrhuTypeSpecInCustomModifiers() |
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.
Typo: "Trhu"
LGTM |
Disallow TypeSpec after CLASS and VALUETYPE in signatures
Addresses issue #7970. This change makes the compiler behave the same as native csc.
Also adds a test for issue #7971.