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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cxx/CXX.h:54
+namespace rules {
+namespace dummy {
+enum Rule {
----------------
usaxena95 wrote:
> sammccall wrote:
> > 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?
> I would include this block in the clang-format off block to show these are for the generated code.
> ```
> //clang-format off
> namespace dummy { enum Rule {
> ...
> };}
> //clang-format on
> ```
ah, I get it (until I read the preprocessed CXX.h code) -- I found it really hard to understand it by literally reading the code here. 

To make it clear,  I think we can probably add two additional RULE_BEGIN, RULE_END macros? 

 in `CXXSymbols.inc`, we emit something like

```
RULE_BEGIN(_)
RULE(_, translation_unit, 135)
RULE(_, statement_seq, 136)
RULE(_, declaration_seq, 137))
RULE_END(_)
```

In CXX.h, we write

```
#define RULE_BEGIN(LHS) namespace LHS { enum Rule : RULE ID {
#define RULE_END()  }; }
```



================
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
----------------
sammccall wrote:
> 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)
> (Note that it's incorrectly indented, and the indentation rules around switch/case are one of the main reasons I find nested switches confusing)

Ah the indentation is weird (it is not what I originally written..). There are ways to make the indentation correct (turn off the clang-format etc).

If nested switch is hard to read, maybe we can wrap one with a macro to improve the code readability (happy to explore ideas, I think there is value on this pattern).


================
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"
----------------
sammccall wrote:
> 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
yeah, that looks good (that means lowercase is always referring to nonterminals)


================
Comment at: clang-tools-extra/pseudo/lib/grammar/Grammar.cpp:62
 
 std::string Grammar::mangleRule(RuleID RID) const {
   const auto &R = lookupRule(RID);
----------------
sammccall wrote:
> 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
ok, feel free to move it.


================
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]);
----------------
sammccall wrote:
> 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.
>  I find the double-underscore version much more readable (than either variant with numbers)

I agree that the double-underscore is more readable, but I don't have a much-better feeling. I'm ok with the double-underscore one if you feel strong about it.

The reason why I value the index is that it is easier to spot the index and its corresponding RHS element, and it could potentially avoid writing bugs (specially in the guard function implementation,
thinking about accessing the RHS element in the looong rule `l_paren__r_paren__decl_specifier_seq__noexcept_specifier__trailing_return_type__requires_clause`).




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130414