[PATCH] D81157: Propose naming principle for NodeRole and apply it

[Dmitri Gribenko via Phabricator via cfe-commits]

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