[PATCH] D129222: [pseudo] Implement a guard to determine function declarator.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/pseudo/gen/Main.cpp:114
+      const clang::pseudo::Rule &R = G.table().Rules[RID];
+      // lhs$$rhs$rhs$rhs
+      std::string EnumName = symbolName(R.Target, G) + "$";
----------------
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > so the dollar signs are a practical problem: at least on my machine the warning is on by default, so we emit thousands of warnings and it's impossible to find the error among them.
> > > 
> > > At *minimum* we need to use pragmas or something to suppress the warning.
> > > 
> > > But using nonstandard C++ seems likely to cause other problems and limit portability for not-great reasons.
> > > 
> > > Some ideas:
> > >  -`Rule::ptr_declarator__EQUALS__ptr_operator__ptr_declarator` (this violates the internal `__` rule, though that one is widely ignored)
> > >  - `Rule::ptr_declarator_0ptr_operator_1ptr_declarator`
> > >  - `Rule::PtrDeclarator_EQ_PtrOperator_PtrDeclarator` (yet another spelling variation, but quite readable. We could even change the spellings in the BNF file...)
> > > so the dollar signs are a practical problem: at least on my machine the warning is on by default, so we emit thousands of warnings and it's impossible to find the error among them.
> > 
> > I didn't see this warning on my machine, I'm using the `g++`.
> > 
> > 
> > > Rule::ptr_declarator__EQUALS__ptr_operator__ptr_declarator (this violates the internal __ rule, though that one is widely ignored)
> > 
> > It is not quite readable to me. 
> > 
> > > Rule::PtrDeclarator_EQ_PtrOperator_PtrDeclarator (yet another spelling variation, but quite readable. We could even change the spellings in the BNF file...)
> > 
> > It is better, but I'm not willing to change the spelling to CamelCase in bnf grammar, I like the current `ptr-declarator` name, and it matches the standard style. We could keep them unchanged in the bnf, and do the conversion during the mangling.
> > 
> > > Rule::ptr_declarator_0ptr_operator_1ptr_declarator
> > 
> > I'm starting to like it now. I find that encoding the index for RHS is useful, especially when we  do some work on the ForestNode based on the rules. So I will vote this one.
> > 
> > I didn't see this warning on my machine, I'm using the g++.
> 
> Clang seems to enable -Wdollar-in-identifier-extension by default.
> 
> > It is better, but I'm not willing to change the spelling to CamelCase in bnf grammar
> 
> Fair enough. I find this option the most readable, but the lack of consistency is annoying.
> 
> > I'm starting to like it now. I find that encoding the index for RHS is useful, especially when we do some work on the ForestNode based on the rules. So I will vote this one.
> 
> Ok, i find this least readable, but good in all other respects. Let's go for it.
Sounds good, separated it in https://reviews.llvm.org/D129359.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129222