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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cxx/CXX.h:54
+namespace rules {
+namespace dummy {
+enum Rule {
----------------
hokein wrote:
> why there is a dummy namespace here?
for each NT, we close the previous ns+enum and open new ones.
For this to work for the first NT, we have to open an ns+enum.

I can add a comment, but would prefer to do this some other way?


================
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
----------------
hokein wrote:
> 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:
>              ....
>       }
>    ...
>    }
> }
> ```
I guess this is a question of taste, but I find that style very hard to read.

(Note that it's incorrectly indented, and the indentation rules around switch/case are one of the main reasons I find nested switches confusing)


================
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"
----------------
hokein wrote:
> IMO, this improves the readability, and also aligns with the text in the cxx.grammar.
Thanks. I like this change too. We still have `semi` vs `;` (should we use `SEMI`?) but it feels clearer


================
Comment at: clang-tools-extra/pseudo/lib/grammar/Grammar.cpp:62
 
 std::string Grammar::mangleRule(RuleID RID) const {
   const auto &R = lookupRule(RID);
----------------
hokein wrote:
> nit: update the doc comment in .h file.
Honestly I would rather just move the impl back into gen - this change seems to demonstrate why


================
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]);
----------------
hokein wrote:
> 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.
Short version: I can deal with the numbers at the front (there's a *little* bit of value), but at the back of the symbol name there's no value, just noise. I find the double-underscore version much more readable (than either variant with numbers).

---

I always found the indexes aesthetically ugly but OK when you could read them as
```
lhs _   0 rhsa_ 1 rhsb
lhs := [0]rhsa [1]rhsb
```

But an identifier can't start with a number (`rule::empty_statement::0semi`) this isn't possible. I think you suggested shifting the numbers to the end, but I can't find a way to read that, it adds too much visual noise. (e.g. because the number "decorations" don't line up in a column in code completion or a switch statement).

I also think the double-underscore version is significantly less cryptic for new readers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130414