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