[PATCH] D121150: [pseudo][WIP] Implement a GLR parser.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

This looks really good, great job on the signatures & tests.
There are a few different ways to formulate the signatures of glrParse/glrReduce, and some possible optimizations, but I can't see anything that's an obvious improvement worth holding up over.



================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:71
+    LRTable::StateID State;
+    // Number of the parents of this node, which are stored as trailing objects.
+    // The parents hold previous parsed symbols, and may resume control after
----------------
nit: *pointers* are stored as trailing objects, not the parents themselves


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:143
+// parsing process (glrShift, or glrReduce).
+using NewHeadCallback =
+    std::function<void(LRTable::StateID NextState, const ForestNode *Parsed,
----------------
nit: this is always used synchronously, so llvm::function_ref?


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:146
+                       llvm::ArrayRef<const GSS::Node *> Parents)>;
+// Apply all PendingShift actions on a given GSS state, newly-created heads are
+// passed to the CB.
----------------
the comment says newly-created heads are passed, but actually their inputs are passed and the callback is responsible for creating them.

(Why not have glrShift create the node and pass it to the callback? Or maybe even pass the output vector of NewHeads& in for concreteness?)


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:147
+// Apply all PendingShift actions on a given GSS state, newly-created heads are
+// passed to the CB.
+//
----------------
Maybe also mention the interaction with PendingShift?

"When this function returns, PendingShift is empty."?


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:151
+void glrShift(std::vector<ParseStep> &PendingShift, const ForestNode &NextTok,
+              const ParseParams &Params, NewHeadCallback CB);
+// Applies PendingReduce actions, until no more reduce actions are available.
----------------
nit: semantics of the callback aren't obvious, so I think "NewHead" is a better name than "CB"


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:153
+// Applies PendingReduce actions, until no more reduce actions are available.
+//
+// Exposed for testing only.
----------------
When this function returns, PendingReduce is empty. Calls to NewHeadCB may add elements to PendingReduce


================
Comment at: clang-tools-extra/pseudo/tool/ClangPseudo.cpp:39
                       desc("Print directive structure of source code"));
+static opt<bool> PrintStats("print-stats", desc("Print GLR parser statistics"));
+static opt<bool> PrintForest("print-forest", desc("Print parse forest"));
----------------
nit: just "print statistics"? I think this should be orthogonal to other options


================
Comment at: clang-tools-extra/pseudo/unittests/GLRTest.cpp:34
+
+struct NewHeadResult {
+  LRTable::StateID State;
----------------
this looks so much like a GSS node: why not just use a GSS node?


================
Comment at: clang-tools-extra/pseudo/unittests/GLRTest.cpp:137
+  glrShift(PendingShift, SemiTerminal, {*G, T, Arena, GSS},
+           std::bind(&GLRTest::captureNewHeads, this, _1, _2, _3));
+
----------------
bind is ugly :-(

maybe just have GLRTest::captureNewHeads() return a std::function with the right signature?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121150