Skip to content

ctbuilder: Parse lexerkind from grmtools section#551

Merged
ltratt merged 1 commit into
softdevteam:masterfrom
ratmice:header_lexerkind
Apr 14, 2025
Merged

ctbuilder: Parse lexerkind from grmtools section#551
ltratt merged 1 commit into
softdevteam:masterfrom
ratmice:header_lexerkind

Conversation

@ratmice
Copy link
Copy Markdown
Collaborator

@ratmice ratmice commented Apr 12, 2025

This adds lexerkind parsing to CTLexerBuilder.

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?

@ratmice
Copy link
Copy Markdown
Collaborator Author

ratmice commented Apr 13, 2025

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.

@ltratt
Copy link
Copy Markdown
Member

ltratt commented Apr 14, 2025

This looks fine to me and I think we can merge it as-is?

@ratmice
Copy link
Copy Markdown
Collaborator Author

ratmice commented Apr 14, 2025

I think so, will have to revisit the parsing code but it's good to get the feature and it's tests in while that is being redone.

@ltratt ltratt added this pull request to the merge queue Apr 14, 2025
Merged via the queue into softdevteam:master with commit e64b15c Apr 14, 2025
2 checks passed
@ratmice ratmice deleted the header_lexerkind branch April 14, 2025 09:44
@ratmice ratmice mentioned this pull request Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants