[PATCH] D125006: [pseudo] Support parsing variant target symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein marked 3 inline comments as done.
hokein added inline comments.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:125
 };
-// Parse the given token stream with the GLR algorithm, and return a forest node
-// of the start symbol.
----------------
sammccall wrote:
> From the caller's perspective, is "target symbol" really a clearer name than "start symbol"? The latter seems like more common terminology.
I'd like to call it `start symbol` as well, it is clearer.

The only downside is that we're diverging from the standard LR literature (`_` is the start symbol of the augmented grammar). We need a name for the symbol `_`, I rename the `Grammar::startSymbol` to `Grammar::underscore` (or even `_`?), please take a look on this patch again.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Grammar.h:188
+// Name must be a valid nonterminal symbol name in the grammar.
+SymbolID findNonterminal(llvm::StringRef Name,
+                         const clang::pseudo::GrammarTable &GT);
----------------
it is unfortunate that we put this function in the Grammar.h. This could be avoided when we have the generated enum grammar type, but we are not there yet...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125006