-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Don't hoist ref locals #43463
Don't hoist ref locals #43463
Conversation
Glad it worked :-) |
I poked around at some linked issues and didn't see others that seemed affected. I think this only resolves issues where either:
|
"; | ||
|
||
var comp = CreateCompilation(source, options: TestOptions.DebugDll); | ||
comp.VerifyEmitDiagnostics(); |
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.
comp.VerifyEmitDiagnostics(); [](start = 12, length = 29)
Consider running the scenario and observing expected behavior at runtime. #Closed
var comp = CreateCompilation(source, options: TestOptions.DebugDll); | ||
comp.VerifyEmitDiagnostics(); | ||
comp = CreateCompilation(source, options: TestOptions.ReleaseDll); | ||
comp.VerifyEmitDiagnostics(); |
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.
comp.VerifyEmitDiagnostics(); [](start = 12, length = 29)
Here as well. #Closed
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.
LGTM (iteration 2), modulo suggestion to strengthen tests.
@RikkiGibson Here are some issues I had in mind:
They also involve ref locals in async methods, but those aren't crashes, so unlikely the same issue :-( |
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.
LGTM Thanks (iteration 4)
{ | ||
public async Task Step(Task<int> t) | ||
{ | ||
ReadMemory() = await t; |
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.
Consider adding a test where we have a ref
l-value and use a compound operator. Essentially
ReadMemory() += await t;
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.
LGTM (iteration 5)
Closes #40251
@gafter could you please take a look? Without this change, we crash in debug mode, while giving the "expected" behavior (either giving an emit diagnostic or compiling successfully) in release mode.