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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
