[PATCH] D129222: [pseudo] Implement a guard to determine function declarator.

[Sam McCall via Phabricator via cfe-commits]

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