Classify pattern-matching temp as long-lived #18756
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@agocke @AlekseyTs May I please have a review? |
|
Approved pending sign offs. |
jaredpar
approved these changes
Apr 18, 2017
gafter
added a commit
to gafter/roslyn
that referenced
this pull request
Apr 18, 2017
…ameters. Fixes dotnet#16195 This is a language change for 7.1. See dotnet/csharplang#154. Some tests may fail until dotnet#18756 is integrated.
agocke
approved these changes
Apr 18, 2017
tmat
reviewed
Apr 18, 2017
| <entry offset=""0xbe"" hidden=""true"" /> | ||
| <entry offset=""0xc8"" startLine=""30"" startColumn=""17"" endLine=""30"" endColumn=""23"" /> | ||
| <entry offset=""0xca"" startLine=""32"" startColumn=""5"" endLine=""32"" endColumn=""6"" /> | ||
| <entry offset=""0xba"" startLine=""23"" startColumn=""17"" endLine=""23"" endColumn=""23"" /> |
There was a problem hiding this comment.
The changes in sequence points suggest there are changes in IL. Seems like no other test covers the difference in IL. It would be good to have one.
tmat
reviewed
Apr 18, 2017
| @@ -207,6 +204,11 @@ internal enum SynthesizedLocalKind | |||
| InstrumentationPayload = 34, | |||
|
|
|||
| /// <summary> | |||
| /// Temp created for pattern matching by type. | |||
| /// </summary> | |||
| PatternMatchingTemp = 35, | |||
There was a problem hiding this comment.
I'd remove "Temp" from the name. "Temp" suggests it's temporary, not long lived.
It would also be useful to mention in the comment what value is specifically stored in this variable. Is it the type object we are switching on?
|
@tmat Thanks for the suggestions. I've modified the PR to address both. |
gafter
added
Resolution-Fixed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Customer scenario
Use a switch in an async method, where the switch uses any new features introduced in C# 7.0.
Bugs this fixes:
Fixes #18257
Workarounds, if any
None known.
Risk
Small, as there is no change to code generation, but rather the classification of variables affecting the PDB.
Performance impact
Tiny, if any. Thee is only a slight change to which (existing) compiler code path is taken for these scenarios.
Is this a regression from a previous update?
No, the pattern-matching switch was new in C# 7.0.
Root cause analysis:
We did not test the interaction of pattern-matching with async.
How was the bug found?
Customer reported.