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