Cleanup split between lexer and sliding-text-window #79205
Conversation
| { | ||
| internal readonly SlidingTextWindow TextWindow; | ||
| private List<SyntaxDiagnosticInfo>? _errors; | ||
| protected int LexemeStartPosition; |
There was a problem hiding this comment.
moved to lexer-layer from SlidingTextWindow
| => TextWindow.GetText(LexemeStartPosition, intern: false); | ||
|
|
||
| protected string GetInternedLexemeText() | ||
| => TextWindow.GetText(LexemeStartPosition, intern: true); |
There was a problem hiding this comment.
these .GetText helpers on the TextWindow were used all over the place. MOved to two simple helpers on Lexer for getting either an interned or non-interned chunk of text for the current lexeme.
| => TextWindow.GetText(LexemeStartPosition, intern: true); | ||
|
|
||
| protected int CurrentLexemeWidth | ||
| => this.TextWindow.Position - LexemeStartPosition; |
There was a problem hiding this comment.
this was previously exposed as the (imo) confusingly named SlidingTextWindow.Width.
I tried to ensure that helpers related to lexemes used that term in their name to keep it clear what 'width/count/etc.' things are referring to.
| internal SyntaxTrivia LookupWhitespaceTrivia( | ||
| SlidingTextWindow textWindow, | ||
| int lexemeStartPosition, | ||
| int hashCode) |
There was a problem hiding this comment.
easier to just read as the new function. There was only ever one caller ofr LookupTrivia, and it always used the same generic arg.
| if (this.HasErrors) | ||
| { | ||
| var afterStartDelimiter = TextWindow.LexemeStartPosition + startingQuoteCount; | ||
| var afterStartDelimiter = this.LexemeStartPosition + startingQuoteCount; |
There was a problem hiding this comment.
Reference to textWindow.LexemeStartPosition were mechanically replaced with this.LexemeStartPosition
| } | ||
|
|
||
| info.Text = TextWindow.GetText(intern: true); | ||
| info.Text = this.GetInternedLexemeText(); |
There was a problem hiding this comment.
References to TextWindow.GetText(intern: true) and TextWindow.GetText(intern: false) were mechanically updated to this.GetInternedLexemeText() and this.GetNonInternedLexemeText() mechanically.
|
|
||
| // The max index in charWindow that we will quick scan to. This is either the end of the window | ||
| // or the position of the largest token we'd be willing to quick scan and cache. | ||
| var maxIndexInWindow = Math.Min(charWindow.Length, startIndexInWindow + MaxCachedTokenSize); |
There was a problem hiding this comment.
renamed these all for clarity.
|
@dotnet/roslyn-compiler for another pair of eyes. |
|
@dotnet/roslyn-compiler for another pair of eyes. thanks! |
| _strings = StringTable.GetInstance(); | ||
| _characterWindow = s_windowPool.Allocate(); | ||
| _lexemeStart = 0; | ||
| _characterWindow = new(s_windowPool.Allocate()); |
There was a problem hiding this comment.
nit: I think target-typed new() is disallowed in the compiler layer.
| return _characterWindow; | ||
| } | ||
| } | ||
| public readonly ReadOnlySpan<char> CurrentWindowSpan => _characterWindow.AsSpan(_positionInText - _characterWindowStartPositionInText); |
There was a problem hiding this comment.
It seems that _positionInText and the character window might not always intersect. For example, Advance methods only move the position but not the window. Could it happen that the AsSpan would fail then? E.g., because the offset would be negative (position would be before the window).
There was a problem hiding this comment.
Yes. Good call. I will fix that up.
|
|
||
| for (; i < n; i++) | ||
| // Where we are currently pointing in the charWindow as we read in a character at a time. | ||
| var currentIndex = 0; |
There was a problem hiding this comment.
Can we put this inside the for initializer?
There was a problem hiding this comment.
no. it is read below (it is used to figure out how many characters were actually read out).
| return _characterWindow; | ||
| } | ||
| } | ||
| public readonly ReadOnlySpan<char> CurrentWindowSpan => _characterWindow.AsSpan(_positionInText - _characterWindowStartPositionInText); |
There was a problem hiding this comment.
Yes. Good call. I will fix that up.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Co-authored-by: Jan Jones <[email protected]>
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| { | ||
| get | ||
| { | ||
| return _positionInText > _textEnd |
There was a problem hiding this comment.
This doesn't look like the right check. If position is after end, that's shouldn't cause problems. But the _positionInText - _characterWindowStartPositionInText offset computed below can be negative if _characterWindowStartPositionInText is after _positionInText, so I would expect this check to be something like:
| return _positionInText > _textEnd | |
| return _positionInText < _characterWindowStartPositionInText |
There was a problem hiding this comment.
Both woudl actually cause problems (since AsSpan checks start/end pos). I'll validate both sides :)
4 participants
The general philosophy here is that the SlidingTextWindow is just a representation of characters, and doesn't really care anything about "current lexeme". Instead, that entire concept it contained entirely within the lexer itself.
This helps allow us to tweak these components with less risk of regression (like the recent regression around lexing out a
dotand ensuring it was not a..in a range).