Migrate LexFlags parsing to GrmtoolsSectionParser#553
Conversation
| .join("\n") | ||
| })?; | ||
| let lexerdef = | ||
| LRNonStreamingLexerDef::<LexerTypesT>::new_with_header(&lex_src, header) |
There was a problem hiding this comment.
I didn't get to the trailhead before the defect in this patch came to me,
We aren't using mark_used on the header, but I'm not even sure we can, passing it by value.
merge_from isn't preserving mark_used, it's kind of assuming that all of that will happen after merging.
It isn't exactly clear to me whether we can pass this to LRNonStreamingLexerDef not as an owned value, but as a &mut ref.
Edit: I'll likely (and feel free to) just close this, but there is a bit here in this patch which seems unlikely to change
There was a problem hiding this comment.
FWIW, there are basically two approaches that I can think of,
- the first is passing
&mut Headerin toLexParser, then doing the merge insideLexParser
That is basically how we didYaccParser, but withLexParserthere is the trait involved.
We may need to add an alternatemerge_intowhich uses the merge behavior of the data source, rather than
the data's destination likemerge_from.
Given the traits inLexerDeftraits I'm not absolutely certain we can pull this one off without modifying the trait, because of the additional lifetime, however I haven't yet tried. - the second is essentially doing the merging outside of
LexParserand passingLexFlagsin.
There is a second code path entirely for reading the header from a string without merging.
Then the difference is the first code path usesHeader<Location>while the second usesHeader<Span>.
This approach is nice in that it keepsLocationoutside ofLexParser, and could do the same for
YaccGrammarif we passYaccKindin.
I think the second approach is more in line/semver compatible with prior releases but is perhaps a bit tricky to get right with it's generic types the patch in 189bbfc was a first step in this direction, but It got tricky fairly quickly after that.
There was a problem hiding this comment.
I haven't totally understood the details, but on the face of it (2) does sound like the better route to me. Is this PR the right place to do it or ...?
There was a problem hiding this comment.
I don't think this PR is the right place for it, it actually ends up undoing a lot of what is in this PR.
I do think (1) can go in this PR, it is basically the same approach taken here, I think maybe a good idea is to try the first approach, finishing it off in this PR if only to give us a fallback option that we can hold off on merging. Just in case something goes awry with (2) which I also think seems preferable but also has some complexity trade-offs.
There was a problem hiding this comment.
the first approach is implemented in this PR in 7468581 this didn't require trait modifications at all, and was actually a relatively minor change to the patch.
I'll try and start work on a new branch implementing second approach tomorrow.
| .join("\n") | ||
| })?; | ||
| let lexerdef = | ||
| LRNonStreamingLexerDef::<LexerTypesT>::new_with_header(&lex_src, header) |
There was a problem hiding this comment.
I haven't totally understood the details, but on the face of it (2) does sound like the better route to me. Is this PR the right place to do it or ...?
| } | ||
| } | ||
| LexBuildHeaderError::Header(e) => { | ||
| format!("{}", e) |
There was a problem hiding this comment.
Dumb question: can we not give line number information for the header?
There was a problem hiding this comment.
I think even that would be awkward, e.g. YaccGrammarError assumes spans exist (the least SpansKind assumes there is one span), we could add a SpansKind::None, and YaccGrammarErrorKind::Header(HeaderErrorKind), It feels to me somewhat like throwing the baby out with the bath water though, and e.g. we still have to make changes of similar scope to adding Location. In that sense Location::CommandLine is somewhat type equivalent to not having a span.
|
Might we be ready for squashing with that latest commit? |
|
Sure, but I still want to hold off/try that other approach, because it would seem like if we can keep |
ratmice
force-pushed
the
lrlex_header_lexflags
branch
from
7468581 to
190497c
Compare
|
Think we can close this one, and will start over migrating |
2 participants
So here is what I've managed to come up with for migrating the
LexFlagsparsing code in the%grmtoolssection to use theGrmtoolsSectionParser, with this patch we parse the%grmtoolssection twice.CTParserBuilder, to pull outLexerKind,LRNonStreamingLexerDefconstructors to get and merge theLexFlags.LexParserhas runHeaderhas been lowered to aLexFlags,and the whole
%grmtoolssection has been stripped off, so we don't need to parse it a third time to find the beginningof the lex file's contents.
We do all the merging in step 2, partially because
LexFlagslacks a notion ofMergeBehaviorand that is the point where we still have enough information to merge without introducing multiple parameters likeforcinganddefault.This also kindly isolates the
LexBuildHeaderError(for lack of any good name) out ofLexParser.This allows us to keep around the
new_with_lex_flagsconstructor but simplified a little bit with fewer arguments.One very important thing to note is that this changes the return type of the trait method
from_str.A second thing to note is that I noticed that
from_rulesdoesn't have any method to change theLexFlags, it is still relying on the defaults.A third thing to note is that this may not produce the nicest of errors, but I think we should be able to improve them without breaking semver now.