[PATCH] D130414: [pseudo] Reorganize CXX.h enums

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for exploring the idea, this looks like a good start to me.



================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cxx/CXX.h:54
+namespace rules {
+namespace dummy {
+enum Rule {
----------------
why there is a dummy namespace here?


================
Comment at: clang-tools-extra/pseudo/lib/cxx/CXX.cpp:119
 
-    switch ((cxx::Rule)Declarator->rule()) {
-    case Rule::noptr_declarator_0declarator_id: // reached the bottom
+    switch (Declarator->rule()) {
+    case rule::noptr_declarator::declarator_id: // reached the bottom
----------------
The code of applying the pattern doesn't look much worse to me and it is easier to verify the exhaustiveness by human as well. We might loose some performance (I hope not a lot), but I think it is a good tradeoff (not saying we need do it in this patch).

```
switch (LHSNonterminal(Declarator->rule(), *G)) {
   case cxx::Symbol::noptr_declarator: {
      switch ((rule::noptr_declarator)Declarator->rule())  {
         case rule::noptr_declarator::declarator_id:
             ....
         case rule::noptr_declarator::noptr_declarator__parameters_and_qualifiers:
             ....
      }
   ...
   }
}
```


================
Comment at: clang-tools-extra/pseudo/lib/cxx/CXX.cpp:175
   return {
-      {(RuleID)Rule::function_declarator_0declarator,
        SYMBOL_GUARD(declarator, isFunctionDeclarator(&N))},
----------------
A nice improvement


================
Comment at: clang-tools-extra/pseudo/lib/grammar/Grammar.cpp:51
 #define TOK(X) #X,
-#define KEYWORD(X, Y) #X,
+#define KEYWORD(Keyword, Condition) llvm::StringRef(#Keyword).upper(),
 #include "clang/Basic/TokenKinds.def"
----------------
IMO, this improves the readability, and also aligns with the text in the cxx.grammar.


================
Comment at: clang-tools-extra/pseudo/lib/grammar/Grammar.cpp:62
 
 std::string Grammar::mangleRule(RuleID RID) const {
   const auto &R = lookupRule(RID);
----------------
nit: update the doc comment in .h file.


================
Comment at: clang-tools-extra/pseudo/lib/grammar/Grammar.cpp:64
   const auto &R = lookupRule(RID);
-  std::string MangleName = mangleSymbol(R.Target);
-  for (size_t I = 0; I < R.seq().size(); ++I)
-    MangleName += llvm::formatv("_{0}{1}", I, mangleSymbol(R.seq()[I]));
+  std::string MangleName;
+  for (size_t I = 0; I < R.seq().size(); ++I) {
----------------
nit: we don't allow nullable nonterminals, so the RHS should never be empty. we could just initialize `MangleName` with `mangleSymbol(R.seq()[0])`, and iterate from 1 (then the trailing two `pop_back` is not needed).



================
Comment at: clang-tools-extra/pseudo/lib/grammar/Grammar.cpp:65
+  std::string MangleName;
+  for (size_t I = 0; I < R.seq().size(); ++I) {
+    MangleName += mangleSymbol(R.seq()[I]);
----------------
ok, now we're dropping the index for all RHS symbols. Just want to know your thought about this. Personally, I'd like to keep this information (`noptr_declarator__l_square__constant_expression__r_square` vs `noptr_declarator0_l_square1_constant_expression2_r_square3`) though it makes the name a bit uglier.

> Change mangling of keywords to ALL_CAPS (needed to turn keywords that appear alone on RHS into valid identifiers)

if we add index number to the name, then this change is not required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130414