[PATCH] D125677: [pseudo] Remove the explicit Accept actions.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:85
 
-  if (!PendingAccept.empty()) {
-    LLVM_DEBUG({
-      llvm::dbgs() << llvm::formatv("Accept: {0} accepted result:\n",
-                                             PendingAccept.size());
-      for (const auto &Accept : PendingAccept)
-        llvm::dbgs() << "  - " << G.symbolName(Accept.Head->Payload->symbol())
-                     << "\n";
-    });
-    assert(PendingAccept.size() == 1);
-    return *PendingAccept.front().Head->Payload;
-  }
+  const ForestNode *Result = nullptr;
+  StateID AcceptState = Params.Table.getGoToState(StartState, StartSymbol);
----------------
hokein wrote:
> sammccall wrote:
> > rather than mingling this with the glrReduce, I'd suggest collecting the set of final heads first and then analyzing them afterwards.
> > 
> > This means looping a second time, but I think the logic around recognizing patterns that look like `accept` might grow (e.g. if we want to incorporate some error tolerance)
> that sounds reasonable, but it requires some additional changes (to identify inactive heads in the callback of `glrReduce`):
> 
> - AddSteps now returns true/false whether there is any actions in LRTable[NewHead->state][nextTok];
> - no available action in the acceptable state on the eof token in LRTable (see the change in lr-build-basic.test);
Hmm, I'm not sure whether the conditional or unconditional version is better, and we're really borrowing problems from the future here.

Let's go back to this simplest version, and update the opaque forest node comment below to a FIXME.
(We will need to invoke our generic error-recovery handlers when we reach EOF without reaching accept state, and simulating shifting an eof token may be the best way to reuse the code)


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:86
+  const ForestNode *Result = nullptr;
+  StateID AcceptState = Params.Table.getGoToState(StartState, StartSymbol);
+  glrReduce(PendingReduce, Params, [&](const GSS::Node *NewHead) {
----------------
this line introduces a requirement that the start symbol be a nonterminal - if this is new, can we doc it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125677