[PATCH] D130066: [pseudo] Key guards by RuleID, add guards to literals (and 0).

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

it looks good.



================
Comment at: clang-tools-extra/pseudo/gen/Main.cpp:102
       llvm::StringRef Name = G.table().AttributeValues[EID];
-      assert(!Name.empty());
       Out.os() << llvm::formatv("EXTENSION({0}, {1})\n", Name, EID);
----------------
I think this invariant is still true even in this patch, ExtensionID for empty attribute value is 0, and we start from 1 in the loop.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/grammar/Grammar.h:31
 //
 //    contextual-override := IDENTIFIER [guard=Override]
 //
----------------
nit: update the stale comment.


================
Comment at: clang-tools-extra/pseudo/lib/cxx/CXX.cpp:27
 
-bool guardOverride(llvm::ArrayRef<const ForestNode *> RHS,
-                   const TokenStream &Tokens) {
-  assert(RHS.size() == 1 &&
-         RHS.front()->symbol() == tokenSymbol(clang::tok::identifier));
-  return Tokens.tokens()[RHS.front()->startTokenIndex()].text() == "override";
-}
-bool guardFinal(llvm::ArrayRef<const ForestNode *> RHS,
-                const TokenStream &Tokens) {
-  assert(RHS.size() == 1 &&
-         RHS.front()->symbol() == tokenSymbol(clang::tok::identifier));
-  return Tokens.tokens()[RHS.front()->startTokenIndex()].text() == "final";
-}
-bool guardModule(llvm::ArrayRef<const ForestNode *> RHS,
-                 const TokenStream &Tokens) {
-  return Tokens.tokens()[RHS.front()->startTokenIndex()].text() == "module";
+bool isStringUD(const Token &Tok) { return !Tok.text().endswith("\""); }
+bool isCharUD(const Token &Tok) { return !Tok.text().endswith("'"); }
----------------
nit: not sure the abbreviation of UD is a clearer, took me a while to understand it is for UserDefined, add a comment?


================
Comment at: clang-tools-extra/pseudo/lib/cxx/CXX.cpp:156
 
 llvm::DenseMap<ExtensionID, RuleGuard> buildGuards() {
+#define TOKEN_GUARD(kind, cond)                                                \
----------------
we might probably to consider to split it out from the CXX.cpp at some point in the future. I think currently it is fine.


================
Comment at: clang-tools-extra/pseudo/lib/cxx/CXX.cpp:171
+      {(RuleID)Rule::non_function_declarator_0declarator,
+       SYMBOL_GUARD(declarator, !isFunctionDeclarator(&N))},
+      {(RuleID)Rule::contextual_override_0identifier,
----------------
nit: add a blank line, I think function-declarator guard is different than the contextual-sensitive guard.


================
Comment at: clang-tools-extra/pseudo/tool/ClangPseudo.cpp:48
 static opt<bool> PrintForest("print-forest", desc("Print parse forest"));
+static opt<bool> ForestAbbrev("forest-abbrev", desc("Abbreviate parse forest"),
+                              init(true));
----------------
I like this flag -- I have been wanted for several times.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130066