[PATCH] D130550: [pseudo] Start rules are `_ := start-symbol EOF`, improve recovery.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:74
     bool GCParity;
+    // Have we already used this node for error recovery? (prevents loops)
+    mutable bool Recovered = false;
----------------
hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > haven't look at it deeply -- is this bug related to this eof change? This looks like a different bug in recovery.
> > They're "related" in that they both fix repeated-recovery scenarios.
> > This change fixes that we can hit an infinite loop when applying recovery repeatedly.
> > The eof change fixes that recovery is (erroneously) only applied once at eof.
> > 
> > I hoped to cover them with the same testcase, which tests repeated recovery at EOF. I can extract this change with a separate test if you like, though it will be very similar to the one I have here.
> > This change fixes that we can hit an infinite loop when applying recovery repeatedly.
> 
> I'm more worried about this bug, I think this is an important bug, worth a separate patch to fix it, right now it looks like a join-effort in the eof change. 
> 
> > The eof change fixes that recovery is (erroneously) only applied once at eof.
> 
> Not sure I follow this. I think the eof change is basically to remove a technical debt (avoid the special case and repeated code after main parsing loop).  Am I missing something?
> 
> Not sure I follow this. I think the eof change is basically to remove a technical debt (avoid the special case and repeated code after main parsing loop). Am I missing something?

There's a bug lurking in that tech debt: if recovery does not advance the cursor then we should go around the loop again, but if it happens at eof then (in the old code) there's no loop to go around at all.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130550/new/

https://reviews.llvm.org/D130550