Simplify the code around parsing interpolated string literals.#57945
Simplify the code around parsing interpolated string literals.#57945CyrusNajmabadi merged 6 commits intodotnet:mainfrom
Conversation
src/Compilers/CSharp/Portable/Parser/LanguageParser_InterpolatedString.cs
Outdated
Show resolved
Hide resolved
| // lexical errors | ||
| var info = default(Lexer.TokenInfo); | ||
| tempLexer.ScanInterpolatedStringLiteralTop(interpolations, isVerbatim, ref info, ref error, out closeQuoteMissing); | ||
| tempLexer.ScanInterpolatedStringLiteralTop(interpolations, isVerbatim, ref info, out error, out closeQuoteMissing); |
There was a problem hiding this comment.
ref was bogus here. we never passed data in.
| { | ||
| internal partial class Lexer | ||
| { | ||
| internal readonly struct Interpolation |
There was a problem hiding this comment.
this is a pure move (and the type was made 'readonly').
| } | ||
|
|
||
| private class InterpolatedStringScanner | ||
| private struct InterpolatedStringScanner |
There was a problem hiding this comment.
this was made a struct as it is not copied and only sits on one stack frame.
There was a problem hiding this comment.
Just curious, what enforces "NonCopyable"?
There was a problem hiding this comment.
We have an analyzer that does it :)
| { | ||
| private readonly Lexer _lexer; | ||
| private bool _isVerbatim; | ||
| private readonly bool _isVerbatim; |
There was a problem hiding this comment.
no need for this to mutate as we can just make a fresh scanner in place.
| try | ||
| { | ||
| _isVerbatim = isVerbatimSubstring; | ||
| ScanInterpolatedStringLiteralTop(interpolations, ref info, closeQuoteMissing: out _); |
There was a problem hiding this comment.
the act of recursing directly into sub functions was adding a lot of complexity in teh raw-string-literal side of things. It's much simpler for the interpolation-sub-scanner to just recurse directly into the lexer so that all state is fresh and appropriate to each sub-item scanned.
There was a problem hiding this comment.
note: we have a lot of tests for this area (several of my previous PRs beefed this up a ton).
There was a problem hiding this comment.
So was the old way trying to reuse the same InterpolatedStringScanner instance between the parent and nested interpolated strings? I don't see why else it would work the way it did..
There was a problem hiding this comment.
Correct. But i posit there is anti-value in doing that, esp. given how recursive the processing is of nested interpolations is.
| case '*': | ||
| // check for and scan /* comment */ | ||
| ScanInterpolatedStringLiteralNestedComment(); | ||
| _lexer.ScanMultiLineComment(out _); |
There was a problem hiding this comment.
can (and should) just reuse the actual helpers we have here.
There was a problem hiding this comment.
note: we have a lot of tests for this area (several of my previous PRs beefed this up a ton).
|
Please squash compiler changes |
|
So, squashing is effectively impossible with these PRs. I have currently 8 separate branches where these chnages are flowing. If i squash, then i always conflict flowing into the other branches. Whereas, if hte other branches are built off of commits forked from these commits, then git understands how to flow things in properly (i.e. knowing what is already in the dest branch, and only applying commits not there). This makes for trivial branch consolidation. At the cost of a true branching structure in the commits :( |
I don't think so. Before opening the next PR for review, just rebase it to erase commits from previous PR. |
Extracted from the raw-string-literals work (esp. ongoing work to get raw-interpolated-string-literals working).