Always use the 'input lifetime inside Symbol. #726
Conversation
There was a problem hiding this comment.
I'm a bit surprised by the original pull request, as the original issue seemed to conclude that it's an edge case without actually much practical motivation, and that it would probably be fine to not do anything about empty grammars. I don't know what other maintainers think of this, but I'm personally a bit reluctant to merge stuff for the sake of merging stuff, if it doesn't have a clear use-case/motivation (that is, the empty grammar seems to be a pathological case, is it worth dealing with it?)
On the other hand, the change looks small and innocuous, so if we decide to go with it, count this review as an approve, as far as the code is concerned.
| @@ -456,7 +456,9 @@ impl<'ascent, 'grammar, W: Write> CodeGenerator<'ascent, 'grammar, W, TableDrive | |||
| .variant_names | |||
| .insert(Symbol::Nonterminal(nt.clone()), name.clone()); | |||
| } | |||
|
|
|||
| if self.grammar.intern_token.is_some() { | |||
There was a problem hiding this comment.
I think this new case deserves a comment describing why we need that (potentially linking to the issue as well). Other steps have a short one describing what happening, and I imagine it's not entirely clear out of the context of the original issue, why we need to do that in the first place.
| @@ -456,7 +456,9 @@ impl<'ascent, 'grammar, W: Write> CodeGenerator<'ascent, 'grammar, W, TableDrive | |||
| .variant_names | |||
| .insert(Symbol::Nonterminal(nt.clone()), name.clone()); | |||
| } | |||
|
|
|||
| if self.grammar.intern_token.is_some() { | |||
| rust!(self.out, "{}phantom_data(::std::marker::PhantomData<&'input ()>),", self.prefix); | |||
There was a problem hiding this comment.
If I understand correctly, this is adding a variant to the corresponding enum to hold phantom data. Would this be sufficient to add this variant only when both self.grammar.nonterminals and self.grammar.terminals are empty, instead of adding unconditionally?
|
I only rebased the patch to see how conflict goes but didn't edit anything including the original issues' comments. @yannham please feel free to take it over if you want to |
|
I'll try my own suggestion when I have a bit time. |
|
I tried it but got a an error about int underflow, as the action code generated for the empty grammar has something like a transition to state |
rebase of #311 because https://github.com/ahmedcharles/lalrpop doesn't exist anymore