gribozavr2 added inline comments.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:94
/// a binary expression'. Used for implementing accessors.
+// How to name NodeRole:
+// If the child node is a token/keyword, end its name with 'Token'/'Keyword'
----------------
I'd suggest to make this into a doc comment, but phrase it in a way that is
useful for users, so that they can understand the pattern too. For example:
Some roles describe parent/child relations that occur multiple times in
language grammar. We define only one role to describe all instances of such
recurring relations. For example, grammar for both "if" and "while" statements
requires an opening paren and a closing paren. The opening paren token is
assigned the `OpenParen` role regardless of whether it appears as a child of
`IfStatement` or `WhileStatement` node. More generally, when grammar requires a
certain fixed token (like a specific keyword, or an opening paren), we define a
role for this token and use it across all grammar rules with the same
requirement. Names of such reusable roles end with a `~Token` or a `~Keyword`
suffix.
Some roles are assigned only to child nodes of one specific parent syntax node
type. Names of such roles start with the name of the parent syntax tree node
type. For example, a syntax node with a role
`BinaryOperatorExpression_leftHandSide` can only appear as a child of a
`BinaryOperatorExpression` node.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:124
IfStatement_thenStatement,
IfStatement_elseKeyword,
IfStatement_elseStatement,
----------------
Shouldn't `elseKeyword` have no prefix?
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:133
TemplateDeclaration_declaration,
ExplicitTemplateInstantiation_externKeyword,
ExplicitTemplateInstantiation_declaration,
----------------
Shouldn't `externKeyword` have no prefix?
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:136
ArraySubscript_sizeExpression,
- TrailingReturnType_arrow,
+ TrailingReturnType_arrowToken,
TrailingReturnType_declarator,
----------------
Shouldn't `arrowToken` have no prefix?
================
Comment at: clang/lib/Tooling/Syntax/Nodes.cpp:124
+ case syntax::NodeRole::LiteralToken:
return OS << "IntegerLiteralExpression_literalToken";
+ case syntax::NodeRole::OperatorExpression_operatorToken:
----------------
Please update the textual representations of roles that you renamed, and
reorder the switch to correspond to the new declaration order.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81157/new/
https://reviews.llvm.org/D81157
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
