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

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas 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'
----------------
gribozavr2 wrote:
> 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.
> 
> 
Thank you a lot. This is really well explained


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:124
   IfStatement_thenStatement,
   IfStatement_elseKeyword,
   IfStatement_elseStatement,
----------------
gribozavr2 wrote:
> Shouldn't `elseKeyword` have no prefix?
When a keyword can only be used by IfStatement, then I think it actually helps readability to have it prepended with the ParentKind. Here everything is nicely grouped, and if someone needs to change IfStatement, it is clear to see where to make the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81157