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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
