Closed adMartem closed 6 months ago
Hi, sorry to be so slow getting back to you. As far as I can tell, this is okay, at least insofar as it working, but I think it's overly complicated. I don't quite understand the block starting at line 83. In particular, the condition on line 85 is always true, no? The parent expansion of an AttemptBlock
is necessarily an ExpansionSequence
, I think. (It could be an ExpansionSequence
with only one child, which is the AttemptBlock
but that is just how things are implemented, a legacy of how things worked way back in ancient times in legacy JavaCC...). So, as far as I can see, the block starting at line 83 could just as well be:
if (parent instanceof AttemptBlock) {
parent = parent.getParent().getParent();
}
And I think line 99 is dead code. And maybe always was? It seems to me that we have to have an instance of ExpansionWithParentheses
here and in that case parent.getParent()
must be an instance of ExpansionSequence
, and so therefore the condition is always false. Or am I wrong about this?
I have to admit that I'm not 100% comfortable with how all this is expressed. I wrote this and I come back to it and fin it confusing! But anyway, could you verify my various conjectures above? (Maybe I'm the one suffering from wooly-headed thinking here...)
Well, I'll have to refresh my memory on this (since I've slept several times). As I recall, however, I tried a simpler version and then discovered that the ATTEMPT blocks can be indefinitely nested, hence the loop. I agree that 99 is dead code, but I wasn't sure of why it was where it was originally.
I'm now thoroughly confused. I thought your suggestion should work, but it doesn't. I also found out that my solution (this PR) only partially works. It allows the up-to-here, but the UTH is ignored in the generated parser code. Essentially the best you can say for it is that it doesn't break anything that is currently working. I'm inclined to close this PR and just leave the issue outstanding, as it isn't affecting me and I doubt anyone else is using ATTEMPT/RECOVER at this point.
If anyone comes around here again, I think this also now manifests as an issue with ENSURE
in the same way. I.e., I would naively assume that if I had an expansion ENSURE (false) NeverGetHere
I could wrap it as ATTEMPT ENSURE (false) NeverGetHere RECOVER {}
with the same result. It just seems to me that ATTEMPT ... RECOVER
should be transparent with respect to lookahead.
See issue #46.