You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This patch contains an interim change which modifies the manual grmtools section parser in LexParser so that it recognizes the lexerkind field, and doesn't produce an error.
In a follow up, the plan is to remove that parser. We are most likely going to have to parse it twice, just so that the second parse can find the position into the source text where the grmtools section ends (Or alternately strip the header off of the source text passed to the LexParser, but I think the former because we don't want users having to do that). But replacing the manual parser + the custom forcing mechanism, and passing the header through seemed like a change that was going to dwarf adding the lexerkind field to the header.
Let me know however if you would prefer those changes to be done all at once instead?
FWIW, I have been having some trouble with the patch after this one (applyting to LexParser the same style of patch as #549, not so much any new or insurmountable problems, more just discontent with it's execution. But I don't think it is anything that affects this patch.
Edit: Thinking on it the only alternative I could think of was making the type of Header generic over the type of Location, or Span, so we could have Header<Span> inside LexParser and Header<Location> inside CTLexerBuilder, then do the call to merge in CTLexerBuilder and pass LexFlags directly to LexParser.
This is also a bit non-linear that we'd use an entirely different code path when building from a string without LexFlags, so essentially we'd never merge under LexParser. I hadn't had the gumption though to try that approach yet, and may just press on, there are also some wrong approaches I have identified that I took with my original LexFlags change that I can rectify so maybe that will help.
This file contains hidden or 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
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.
This adds
lexerkindparsing toCTLexerBuilder.This patch contains an interim change which modifies the manual grmtools section parser in
LexParserso that it recognizes thelexerkindfield, and doesn't produce an error.In a follow up, the plan is to remove that parser. We are most likely going to have to parse it twice, just so that the second parse can find the position into the source text where the grmtools section ends (Or alternately strip the header off of the source text passed to the
LexParser, but I think the former because we don't want users having to do that). But replacing the manual parser + the custom forcing mechanism, and passing the header through seemed like a change that was going to dwarf adding thelexerkindfield to the header.Let me know however if you would prefer those changes to be done all at once instead?