-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix ilasm parsing of defines #42002
Fix ilasm parsing of defines #42002
Conversation
Regression introduced by dotnet#41611 Fixes dotnet#42001
cc @am11 |
Ah, so this is a case of ANSI string, which we decided not to support / care about in ilasm, but it turned out we do need to support it. |
@@ -924,10 +924,6 @@ int yylex() | |||
tok = parse_literal(curSym, curPos, FALSE); | |||
if(tok == QSTRING) | |||
{ | |||
// insert UTF-8 BOM prefix |
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.
Will strings encoded in UTF-8 still work?
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.
I was not able to figure out what this was supposed to help with. Non-ASCII QSTRINGs (quoted strings) never worked in this position as far as I can tell. The following:
#define NAME "ššš"
.assembly NAME
fails in all .NET Framework and earlier .NET Core versions of ilasm. The case that this change is fixing is:
#define NAME "sss"
.assembly NAME
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.
Ok, thanks for the details.
Are we going to backport it to RC2? or Will this be .NET6 fix? |
This is .NET 6 only regression. No backporting necessary. |
Regression introduced by #41611
Fixes #42001