-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Turn StateTableError to an enum, handle self-loop for #290 #294
Conversation
StateTableError previously required a PIdx. At the point we discover this self loop we don't have one. In order to convey this error change StateTableError into an enum, combining the `pidx` field with its kind.
Interestingly if I take your example:
and run it on (traditional-ish) yacc I get:
but on Bison:
In both cases we have a warning, not an error, but the yacc warning is IMHO clearer. So we probably have two questions to think about:
|
I think if we want to turn it into a warning we need to do something about the infinitely growing action vector when it hits that state. (Although I guess my program should probably refuse to try and parse files, unless the number of conflicts to ignore is set appropriately). I'm not exactly sure what the right way to go about I would assume replacing it with State |
We should probably do whatever yacc/bison does, at least if we're in |
I agree, in general with that (It seems like I overlooked the I think at least the way this works in the bison way is by having states in the For now though, I'm just going to work around it by not running generated parsers which have conflicts when the conflict limits are not specified (Should have probably been doing this already). So that i'm not in a rush to fix it, and can take the time to do it correctly. |
Does that mean we should close this PR or ... ? |
Sorry, that's a weird cultural thing of not being terribly direct, I'm fine with closing it if you want to, there is perhaps some useful information here, but that doesn't actually disappear if we do. |
OK, agreed, we might as well close this -- it'll still be here. |
This test case is not caught by the error that I proposed in pr softdevteam#294.
Here is at least an attempt to fix #290,
It appeared to me that bison ignores this case entirely (no additional errors or self recursion, besides the conflicts, and if you run the parser it won't loop forever), and I believe I see in the bison code where it ignores it.
It's worth pointing out the Accept/Reduce errors here with it's "Infinitely recursive rule let through",
I haven't yet gone and tried to fully understand what makes this case different from an Accept/Reduce.
I had to switch the error around here, because unless I am mistaken, at that point we don't appear to have any way to get a
PIdx
.