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

[Haojian Wu via Phabricator via cfe-commits]

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

I think this patch is in a good state.



================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/cxx/CXX.h:54
+namespace rules {
+namespace dummy {
+enum Rule {
----------------
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > 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
> > > > ```
> > > Oops, they're not for the generated code, just for the macro definition (clang-format gets very confused)
> > > 
> > > Restricted the scope of the formatting-disabled block and tried to improve the hand-formatting (though I don't think any formatting of this particular preprocessor trick is very readable)
> > 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()  }; }
> > ```
> > 
> That would make the callsite more clear, but at the cost of adding ad-hoc bits to the CXXSymbols.inc.
> I'd prefer to keep it generic if we can (otherwise we might as well just have Gen generate the enum definitions directly, right?)
> 
> Added a comment to explain the dummy.
> That would make the callsite more clear, but at the cost of adding ad-hoc bits to the CXXSymbols.inc.

I think this is the point. We read this cxx.h more often than the CXXSymbols.inc, this seems to be a right tradeoff to me.

> I'd prefer to keep it generic if we can (otherwise we might as well just have Gen generate the enum definitions directly, right?)
And yeah, generating the whole definitions from the Gen tool is another option.

OK, if you prefer to do it in this way, could you at least rename the `enum Rule {` to something like `enum PlaceHolder {`? It is very confusing to use the meaningful name `Rule` for this dummy enum.


================
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:
> > 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).
> Maybe - I think nonstandard formatting and macros cost a lot of readability in their own right.
> 
> I think we should probably discuss this separately - I think this change is worth having even if we don't change control flow to exploit in in all cases, WDYT?
yeah, it should not block this change.


================
Comment at: clang-tools-extra/pseudo/unittests/GrammarTest.cpp:124
+            "cv_qualifier__ptr_declarator__semi");
+  EXPECT_EQ(G.mangleRule(ruleFor("ptr-declarator")), "star__identifier");
+  EXPECT_EQ(G.mangleRule(ruleFor("cv-qualifier")), "CONST");
----------------
nit: I think the test is not valid now because we don't have mangleRule in the Grammar.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130414