[PATCH] D122139: [pseudo] Introduce parse forest.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Forest.h:35
+// trailing array) and have pointers to them. "Children" has different semantics
+// depending on the node kinds, e.g. for an Ambiguous node, it means all
+// possible interpretations; for a Sequence node, it means each symbol on the
----------------
I'd replace ", e.g." with a period. (There are in fact no other cases, and likely never will be, so "e.g." is a bit misleading)


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Forest.h:39
+//
+// Since this is a node in a DAG, a node may have multiple parents.
+class alignas(class ForestNode *) ForestNode {
----------------
maybe also "Nodes do not have parent pointers"?


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Forest.h:143
+    assert(!Alternatives.empty());
+    assert(llvm::find_if(Alternatives,
+                         [SID](const ForestNode *Alternative) {
----------------
nit: llvm::all_of


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Forest.h:152
+  }
+
+  ForestNode &createOpaque(SymbolID SID, Token::Index Start) {
----------------
nit: either drop this blank line or add them above


================
Comment at: clang-tools-extra/pseudo/unittests/ForestTest.cpp:29
+
+  SymbolID id(llvm::StringRef Name) const {
+    for (unsigned I = 0; I < NumTerminals; ++I)
----------------
I'd prefer id -> symbol, to disambiguate vs rule id


================
Comment at: clang-tools-extra/pseudo/unittests/ForestTest.cpp:40
+
+  RuleID singleRule(llvm::StringRef NonterminalName) const {
+    auto RuleRange = G->table().Nonterminals[id(NonterminalName)].RuleRange;
----------------
nit: I think this would be read more clearly as "ruleFor" or "rule" or at least "onlyRule" to emphasize "rule" more and "single" less.

Combining with previous:

createSequence(symbol("id-expression"), ruleFor("id-expression"), {&T.front()})


================
Comment at: clang-tools-extra/pseudo/unittests/ForestTest.cpp:44
+      return G->table().Nonterminals[id(NonterminalName)].RuleRange.Start;
+    ADD_FAILURE() << NonterminalName << " is expected to have a single rule!";
+    return 0;
----------------
nit: point out the failure more actionably

"singleRule(" << NonTerminalName << ") but " << NonTerminalName << " has " << N << " rules!"



================
Comment at: clang-tools-extra/pseudo/unittests/ForestTest.cpp:65
+  auto T = Arena.createTerminals(TS);
+  ASSERT_EQ(T.size(), 3ul);
+  const auto *Left = &Arena.createSequence(
----------------
not sure what you intend by the `l` here, but unsigned long != size_t in general.
(I think in standard LLVM warning config `3u` is fine)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122139