Migrate LexFlags parsing to GrmtoolsSectionParser and Header.#556
Conversation
|
|
||
| pub(crate) fn default_lex_flags_header() -> Header<Span> { | ||
| // Because this is called in `from_str`, which needs a span to convert to `LexBuildError` | ||
| // there doesn't seem to be any way to avoid making up this span out of thin air. |
There was a problem hiding this comment.
So, One thing we could do here is just remove this function and implement the defaults manually on LexFlags so we don't have to synthesize Span like this.
It unfortunately gets called from a few places, so perhaps pub(crate) fn setup_default_lex_flags(&mut LexFlags).
Edit: I suppose that with that we'll still need to add to the Header in CTLexerBuilder if only so that mark_required will work, however we don't have any required arguments for LRLex, and so we aren't actually checking that.
Anyhow I haven't really formed an opinion on what the right thing to do here is, but lean towards
- pushing the default initialization further down
possibly as far down as the.Regexinitialization inRule - documenting the things which have default initialization such that they should not be marked
required.
There was a problem hiding this comment.
The best I could think of is in 82069cc
We couldn't do this so late as when constructing rules, because they affect parsing of the .l file.
Also I didn't document these shouldn't be marked required, since the constructors already document their
default values it seemed redundant.
| .map_err(|e| format!("When reading '{}': {e}", lexerp.display()))?; | ||
| let mut header = self.header; | ||
| let defaults = default_lex_flags_header(); | ||
| defaults.merge_into(&mut header)?; |
There was a problem hiding this comment.
This was the only call to merge_into so we could remove that function if we want to?
There was a problem hiding this comment.
Might we need it again later or is this likely to be the only user of that function ever?
There was a problem hiding this comment.
I cannot see us needing to call it later, in the sense that I think this series has all the merge_from calls that the repo should currently need. In order to need to call it we would need to add another tool, otherwise it would be that a user needs to call it if we decide to make this pub + unhidden.
I needed it for approximately two attempts of various patch series, there are two reasons merge_into is usable in more cases than merge_from:
- It is more flexible when merging the
DEFAULT_LEX_FLAGSsince it requires no knowledge about the destination header to know that thefromheader will be overridden by the destinations values viaTheirs. - It allows merging the defaults into another structure without moving that struct since it takes
otheras an owned value.
Since we now know the contents of the source header, the first is not necessary,
and since we're not passing headers by mutable reference, the second is no longer necessary.
Thus we currently fall into the more narrow band where merge_from can be used to the same effect.
It is a fairly simple derivative of copy/paste merge_from and reverse the src/destination values so it isn't the end of the world to just add it at that time, if we find we need it either.
There was a problem hiding this comment.
OK, so should we remove it or keep it?
There was a problem hiding this comment.
I think since it is actually introduced in this series, we should remove it if only because the series is too big already. If it was already in the tree I'd probably lean towards leaving it be just for API completeness.
Because of course we can always add it in a separate patch later if it's needed.
|
Please squash. |
This unifies the code in lrlex with the rest of the crates by using the `GrmtoolsSectionParser`, for parsing the `%grmtools` section and using `Header` for merging values from the builder with those from the parsed header.
ratmice
force-pushed
the
lrlex_header_lexflags_on_pr554
branch
from
1a43c84 to
707bfd7
Compare
|
Squashed, I tried to pull out the few free-standing commits I could find. |
|
Okay, well I think unless anything comes up that should conclude that patch series. I don't really have a whole lot in the way of breaking changes left on my todo list, |
|
Looking forward to it! |
2 participants
This rebuilds my previous PR #553 on top of the current master branch.
I didn't manage to avoid all problems associated with Span/Location conversion.
In particular, because we need to synthesize a
LexFlagsfor default values, and merge those in with the%grmtoolssection in the.lfile, we have to synthesize a made up span for said default values.It isn't to difficult to see though that when the
LexFlags::try_frominfrom_strencounters a default value with a synthesized span, the hard coded values in the default shouldn't cause an error, and therefore the span should go unused. After which the header parameter to try_from gets dropped. So as much as I dislike just making up spans, in this case it doesn't really seem like it can materialize into a problem where the user could see the synthesized span.