sammccall added inline comments.
================
Comment at: clang-tools-extra/pseudo/gen/Main.cpp:60
+
+std::string symbolName(clang::pseudo::SymbolID SID,
+ const clang::pseudo::Grammar &G) {
----------------
This code is copied right?
================
Comment at: clang-tools-extra/pseudo/gen/Main.cpp:62
+ const clang::pseudo::Grammar &G) {
+ static const char *const TokNames[] = {
+#define TOK(X) #X,
----------------
this deserves a comment: "Compared to in the grammar, ; becomes semi and goto
becomes kw_goto"?
================
Comment at: clang-tools-extra/pseudo/gen/Main.cpp:113
+ for (clang::pseudo::RuleID RID = 0; RID < G.table().Rules.size(); ++RID) {
+ const clang::pseudo::Rule &R = G.table().Rules[RID];
+ // lhs$$rhs$rhs$rhs
----------------
pull out a ruleName() too?
or mangleSymbol()/mangleRule() by analogy with linker name mangling
================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Language.h:29
// Return true if the guard is satisfied.
using RuleGuard = llvm::function_ref<bool(
+ RuleID RID, llvm::ArrayRef<const ForestNode *> RHS, const TokenStream &)>;
----------------
This allows/encourages guards that dynamically consider multiple rules, which
increases the amount of coupling between rules and grammar.
Can we express this as
function-declarator := declarator [guard=FunctionDeclarator]?
This does mangle the grammar a bit to our implementation, and introduces two
names for the same concept (guard & nonterminal).
If this feels ugly and redundant, another option would be to change the guard
to be specified by the rule ID instead of an extension ID:
```
function-declarator := declarator [guard]
DenseMap<RuleID, RuleGuard> Guards;
```
================
Comment at: clang-tools-extra/pseudo/lib/cxx/CXX.cpp:58
+ // 2) walkup the stack, find the first rule that can determine the kind
+ auto DFS = [&Kind](const ForestNode *Declarator, auto DFS) {
+ // !! The declarator defined in the cxx.bnf should be unambiguous.
----------------
The "dfs" doesn't actually branch anywhere, so it's just a linear walk, and I
think replacing the recursion with iteration is actually more readable here.
Also, the first known kind on the walk up == the last known kind on the walk
down.
So I think this can be written as:
```
bool IsFunction = false;
// Walk down the declarator chain, innermost one wins.
// e.g. (*x)() is a non function, but *(x()) is a function.
for (;;) {
if (Declarator->kind() != Sequence) // not well-formed, guess
return IsFunction;
switch (Declarator->rule()) {
case noptr_declarator$$declarator_id: // reached the bottom
return IsFunction;
// *X is a nonfunction (unless X is a function).
case ptr_declarator$$ptr_operator$ptr_declarator:
Declarator = Declarator->elements()[1];
IsFunction = false;
continue;
// X() is a function (unless X is a pointer or similar)
case declarator$$noptr_declarator$parameters_and_qualifiers$...:
Declarator = Declarator->elements()[0];
IsFunction = true;
continue;
// (X) is whatever X is.
case declarator$$l_paren$ptr_declarator$r_paren:
Declarator = Declarator->elements()[1];
continue;
// ... more cases ...
default:
assert(false && "unhandled declarator for IsFunction");
return IsFunction;
}
}
```
================
Comment at: clang-tools-extra/pseudo/lib/cxx/CXX.cpp:60
+ // !! The declarator defined in the cxx.bnf should be unambiguous.
+ // FIXME: should we consider opaque node?
+ assert(Declarator->kind() == ForestNode::Sequence);
----------------
yes, we should. We can't descend, so I think we should just treat it as if it
were an identifier.
================
Comment at: clang-tools-extra/pseudo/lib/cxx/CXX.cpp:108
+ }
+ };
+ DFS(DeclaratorFN, DFS);
----------------
if you haven't handled all rule kinds, you're just falling off the end here?
We should have an assertion in debug mode and probably treat it as opaque in
production I think
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129222/new/
https://reviews.llvm.org/D129222
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
