use simpler GrmtoolsSectionValue throughout - #666
Conversation
Sorry, something went wrong.
| impl<'input> GrmtoolsSectionParser<'input> { | ||
| fn parse_setting(&'_ self, mut i: usize) -> Result<(Setting<Span>, usize), HeaderError<Span>> { | ||
| fn parse_setting( | ||
| &'_ self, |
There was a problem hiding this comment.
Just noticing that now a few of the functions like parse_setting and parse_namespaced which
historically made sense since they previously returned Setting and Namespaced probably need better names like parse_rustlike for parse_namespaced..
Alas I don't have a good idea for parse_setting, which is to the right hand side of the : in value: setting.
Sorry, something went wrong.
Sorry, something went wrong.
| member: ("None".to_string(), from_loc.clone()), | ||
| })), | ||
| RecoveryKind::CPCTPlus => { | ||
| GrmtoolsSectionValue::RustLike("RecoveryKind::CPCTPlus".to_string(), from_loc) |
There was a problem hiding this comment.
I think these can probably be simplified by using the ToTokens/implementations to produce the string
rather than these hard coded values.
Ditto for elsewhere.
Sorry, something went wrong.
There was a problem hiding this comment.
Alas, that doesn't seem to work (it emits a fully qualified path like :: lrpar :: RecoveryKind :: CPCTPlus)
Sorry, something went wrong.
There was a problem hiding this comment.
The best I could come up with to simplify these further was using Debug 3129318
Sorry, something went wrong.
This is an experiment to see if we can do away with Value<T> and all it's friends Setting, Namespaced, etc.
Replacing it with the much simpler GrmtoolsSectionValue<T>. This can lead to slightly worse error messages.
e.g. because we're checking YaccKind as a whole including YaccOriginalActionKind. But I think it is pretty much limited to that. We could perhaps do better, by attempting to parse the RustLike(string) rather than the simple matching I've done here.
Sadly this patch has ended up kind of redoing some of the case-insensitivity migration in #665
This came up because when experimenting with integrading the lookup methods with the used value checking.
This involved changing the value owned by the GrammarAST from a HashMap<GrmtoolsSectionValue> to the Header<T>. Then the lookup methods would have to clone the value and return an owned GrmtoolsSectionValue, because there was none to borrow anymore.
It feels like this solves that borrowing issue, but also is a much simpler structure, and so cleans up the code a lot?