Propose naming principle for NodeRole and apply it

[gribozavr2]

clang/include/clang/Tooling/Syntax/Nodes.h

94

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.

140

Shouldn't elseKeyword have no prefix?

148–149

Shouldn't externKeyword have no prefix?

150–151

Shouldn't arrowToken have no prefix?

clang/lib/Tooling/Syntax/Nodes.cpp

128–129

Please update the textual representations of roles that you renamed, and reorder the switch to correspond to the new declaration order.

[eduucaldas]

Fix cout, add explanation

clang/include/clang/Tooling/Syntax/Nodes.h

94

Thank you a lot. This is really well explained

140

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.

[gribozavr2]

clang/include/clang/Tooling/Syntax/Nodes.h

140

OK, makes sense.

[eduucaldas]

ArrowToken, ExternKeyword

[gribozavr2]

clang/include/clang/Tooling/Syntax/Nodes.h

110

There are some leftovers of old text after the period.

clang/lib/Tooling/Syntax/Nodes.cpp

155

Please reorder.

[eduucaldas]

fixed

clang/include/clang/Tooling/Syntax/Nodes.h

enum class NodeKind : uint16_t {
   ParametersAndQualifiers,
   MemberPointer
 };
 /// For debugging purposes.
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, NodeKind K);
 
 /// A relation between a parent and child node, e.g. 'left-hand-side of
 /// a binary expression'. Used for implementing accessors.
+///
+/// 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.
 enum class NodeRole : uint8_t {
   // Roles common to multiple node kinds.
   /// A node without a parent
   Detached,
   /// Children of an unknown semantic nature, e.g. skipped tokens, comments.
   Unknown,
   /// An opening parenthesis in argument lists and blocks, e.g. '{', '(', etc.
   OpenParen,
   /// A closing parenthesis in argument lists and blocks, e.g. '}', ')', etc.
   CloseParen,
   /// A keywords that introduces some grammar construct, e.g. 'if', 'try', etc.
   IntroducerKeyword,
+  /// A token that represents a literal, e.g. 'nullptr', '1', 'true', etc.
+  LiteralToken,
+  /// Tokens or Keywords
+  ArrowToken,
+  ExternKeyword,
   /// An inner statement for those that have only a single child of kind
   /// statement, e.g. loop body for while, for, etc; inner statement for case,
   /// default, etc.
   BodyStatement,
 
   // Roles specific to particular node kinds.
-  UnaryOperatorExpression_operatorToken,
+  OperatorExpression_operatorToken,
   UnaryOperatorExpression_operand,
   BinaryOperatorExpression_leftHandSide,
-  BinaryOperatorExpression_operatorToken,
   BinaryOperatorExpression_rightHandSide,
-  CxxNullPtrExpression_keyword,
-  IntegerLiteralExpression_literalToken,
   CaseStatement_value,
   IfStatement_thenStatement,
   IfStatement_elseKeyword,
   IfStatement_elseStatement,
   ReturnStatement_value,
   ExpressionStatement_expression,
   CompoundStatement_statement,
   StaticAssertDeclaration_condition,
   StaticAssertDeclaration_message,
   SimpleDeclaration_declarator,
   TemplateDeclaration_declaration,
-  ExplicitTemplateInstantiation_externKeyword,
   ExplicitTemplateInstantiation_declaration,
   ArraySubscript_sizeExpression,
-  TrailingReturnType_arrow,
   TrailingReturnType_declarator,
   ParametersAndQualifiers_parameter,
   ParametersAndQualifiers_trailingReturn
 };
 /// For debugging purposes.
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, NodeRole R);
 
 class SimpleDeclarator;
 

clang/lib/Tooling/Syntax/BuildTree.cpp

public:
   bool WalkUpFromExpr(Expr *E) {
     assert(!isImplicitExpr(E) && "should be handled by TraverseStmt");
     Builder.foldNode(Builder.getExprRange(E),
                      new (allocator()) syntax::UnknownExpression, E);
     return true;
   }
 
   bool WalkUpFromIntegerLiteral(IntegerLiteral *S) {
-    Builder.markChildToken(
-        S->getLocation(),
-        syntax::NodeRole::IntegerLiteralExpression_literalToken);
+    Builder.markChildToken(S->getLocation(), syntax::NodeRole::LiteralToken);
     Builder.foldNode(Builder.getExprRange(S),
                      new (allocator()) syntax::IntegerLiteralExpression, S);
     return true;
   }
 
   bool WalkUpFromCXXNullPtrLiteralExpr(CXXNullPtrLiteralExpr *S) {
-    Builder.markChildToken(S->getLocation(),
-                           syntax::NodeRole::CxxNullPtrExpression_keyword);
+    Builder.markChildToken(S->getLocation(), syntax::NodeRole::LiteralToken);
     Builder.foldNode(Builder.getExprRange(S),
                      new (allocator()) syntax::CxxNullPtrExpression, S);
     return true;
   }
 
   bool WalkUpFromUnaryOperator(UnaryOperator *S) {
-    Builder.markChildToken(
-        S->getOperatorLoc(),
-        syntax::NodeRole::UnaryOperatorExpression_operatorToken);
+    Builder.markChildToken(S->getOperatorLoc(),
+                           syntax::NodeRole::OperatorExpression_operatorToken);
     Builder.markExprChild(S->getSubExpr(),
                           syntax::NodeRole::UnaryOperatorExpression_operand);
 
     if (S->isPostfix())
       Builder.foldNode(Builder.getExprRange(S),
                        new (allocator()) syntax::PostfixUnaryOperatorExpression,
                        S);
     else
       Builder.foldNode(Builder.getExprRange(S),
                        new (allocator()) syntax::PrefixUnaryOperatorExpression,
                        S);
 
     return true;
   }
 
   bool WalkUpFromBinaryOperator(BinaryOperator *S) {
     Builder.markExprChild(
         S->getLHS(), syntax::NodeRole::BinaryOperatorExpression_leftHandSide);
-    Builder.markChildToken(
-        S->getOperatorLoc(),
-        syntax::NodeRole::BinaryOperatorExpression_operatorToken);
+    Builder.markChildToken(S->getOperatorLoc(),
+                           syntax::NodeRole::OperatorExpression_operatorToken);
     Builder.markExprChild(
         S->getRHS(), syntax::NodeRole::BinaryOperatorExpression_rightHandSide);
     Builder.foldNode(Builder.getExprRange(S),
                      new (allocator()) syntax::BinaryOperatorExpression, S);
     return true;
   }
 
   bool WalkUpFromCXXOperatorCallExpr(CXXOperatorCallExpr *S) {
     if (S->isInfixBinaryOp()) {
       Builder.markExprChild(
           S->getArg(0),
           syntax::NodeRole::BinaryOperatorExpression_leftHandSide);
       Builder.markChildToken(
           S->getOperatorLoc(),
-          syntax::NodeRole::BinaryOperatorExpression_operatorToken);
+          syntax::NodeRole::OperatorExpression_operatorToken);
       Builder.markExprChild(
           S->getArg(1),
           syntax::NodeRole::BinaryOperatorExpression_rightHandSide);
       Builder.foldNode(Builder.getExprRange(S),
                        new (allocator()) syntax::BinaryOperatorExpression, S);
       return true;
     }
     return RecursiveASTVisitor::WalkUpFromCXXOperatorCallExpr(S);
if (ReturnDeclaratorRange.isValid()) {
                        ReturnDeclarator, nullptr);
     }
 
     // Build node for trailing return type.
     auto Return = Builder.getRange(ReturnedType.getSourceRange());
     const auto *Arrow = Return.begin() - 1;
     assert(Arrow->kind() == tok::arrow);
     auto Tokens = llvm::makeArrayRef(Arrow, Return.end());
-    Builder.markChildToken(Arrow, syntax::NodeRole::TrailingReturnType_arrow);
+    Builder.markChildToken(Arrow, syntax::NodeRole::ArrowToken);
     if (ReturnDeclarator)
       Builder.markChild(ReturnDeclarator,
                         syntax::NodeRole::TrailingReturnType_declarator);
     auto *R = new (allocator()) syntax::TrailingReturnType;
     Builder.foldNode(Tokens, R, L);
     return R;
   }
 
   void foldExplicitTemplateInstantiation(
       ArrayRef<syntax::Token> Range, const syntax::Token *ExternKW,
       const syntax::Token *TemplateKW,
       syntax::SimpleDeclaration *InnerDeclaration, Decl *From) {
     assert(!ExternKW || ExternKW->kind() == tok::kw_extern);
     assert(TemplateKW && TemplateKW->kind() == tok::kw_template);
-    Builder.markChildToken(
-        ExternKW,
-        syntax::NodeRole::ExplicitTemplateInstantiation_externKeyword);
+    Builder.markChildToken(ExternKW, syntax::NodeRole::ExternKeyword);
     Builder.markChildToken(TemplateKW, syntax::NodeRole::IntroducerKeyword);
     Builder.markChild(
         InnerDeclaration,
         syntax::NodeRole::ExplicitTemplateInstantiation_declaration);
     Builder.foldNode(
         Range, new (allocator()) syntax::ExplicitTemplateInstantiation, From);
   }
 

clang/lib/Tooling/Syntax/Nodes.cpp

llvm::raw_ostream &syntax::operator<<(llvm::raw_ostream &OS, NodeRole R) {
   case syntax::NodeRole::Unknown:
     return OS << "Unknown";
   case syntax::NodeRole::OpenParen:
     return OS << "OpenParen";
   case syntax::NodeRole::CloseParen:
     return OS << "CloseParen";
   case syntax::NodeRole::IntroducerKeyword:
     return OS << "IntroducerKeyword";
+  case syntax::NodeRole::LiteralToken:
+    return OS << "LiteralToken";
+  case syntax::NodeRole::ArrowToken:
+    return OS << "ArrowToken";
+  case syntax::NodeRole::ExternKeyword:
+    return OS << "ExternKeyword";
   case syntax::NodeRole::BodyStatement:
     return OS << "BodyStatement";
   case syntax::NodeRole::CaseStatement_value:
     return OS << "CaseStatement_value";
   case syntax::NodeRole::IfStatement_thenStatement:
     return OS << "IfStatement_thenStatement";
   case syntax::NodeRole::IfStatement_elseKeyword:
     return OS << "IfStatement_elseKeyword";
   case syntax::NodeRole::IfStatement_elseStatement:
     return OS << "IfStatement_elseStatement";
-  case syntax::NodeRole::IntegerLiteralExpression_literalToken:
-    return OS << "IntegerLiteralExpression_literalToken";
-  case syntax::NodeRole::CxxNullPtrExpression_keyword:
-    return OS << "CxxNullPtrExpression_keyword";
-  case syntax::NodeRole::UnaryOperatorExpression_operatorToken:
-    return OS << "UnaryOperatorExpression_operatorToken";
+  case syntax::NodeRole::OperatorExpression_operatorToken:
+    return OS << "OperatorExpression_operatorToken";
   case syntax::NodeRole::UnaryOperatorExpression_operand:
     return OS << "UnaryOperatorExpression_operand";
   case syntax::NodeRole::BinaryOperatorExpression_leftHandSide:
     return OS << "BinaryOperatorExpression_leftHandSide";
-  case syntax::NodeRole::BinaryOperatorExpression_operatorToken:
-    return OS << "BinaryOperatorExpression_operatorToken";
   case syntax::NodeRole::BinaryOperatorExpression_rightHandSide:
     return OS << "BinaryOperatorExpression_rightHandSide";
   case syntax::NodeRole::ReturnStatement_value:
     return OS << "ReturnStatement_value";
   case syntax::NodeRole::ExpressionStatement_expression:
     return OS << "ExpressionStatement_expression";
   case syntax::NodeRole::CompoundStatement_statement:
     return OS << "CompoundStatement_statement";
   case syntax::NodeRole::StaticAssertDeclaration_condition:
     return OS << "StaticAssertDeclaration_condition";
   case syntax::NodeRole::StaticAssertDeclaration_message:
     return OS << "StaticAssertDeclaration_message";
   case syntax::NodeRole::SimpleDeclaration_declarator:
     return OS << "SimpleDeclaration_declarator";
   case syntax::NodeRole::TemplateDeclaration_declaration:
     return OS << "TemplateDeclaration_declaration";
-  case syntax::NodeRole::ExplicitTemplateInstantiation_externKeyword:
-    return OS << "ExplicitTemplateInstantiation_externKeyword";
   case syntax::NodeRole::ExplicitTemplateInstantiation_declaration:
     return OS << "ExplicitTemplateInstantiation_declaration";
   case syntax::NodeRole::ArraySubscript_sizeExpression:
     return OS << "ArraySubscript_sizeExpression";
-  case syntax::NodeRole::TrailingReturnType_arrow:
-    return OS << "TrailingReturnType_arrow";
   case syntax::NodeRole::TrailingReturnType_declarator:
     return OS << "TrailingReturnType_declarator";
   case syntax::NodeRole::ParametersAndQualifiers_parameter:
     return OS << "ParametersAndQualifiers_parameter";
   case syntax::NodeRole::ParametersAndQualifiers_trailingReturn:
     return OS << "ParametersAndQualifiers_trailingReturn";
   }
   llvm_unreachable("invalid role");
 }
 
 syntax::Leaf *syntax::IntegerLiteralExpression::literalToken() {
   return llvm::cast_or_null<syntax::Leaf>(
-      findChild(syntax::NodeRole::IntegerLiteralExpression_literalToken));
+      findChild(syntax::NodeRole::LiteralToken));
 }
 
 syntax::Leaf *syntax::CxxNullPtrExpression::nullPtrKeyword() {
   return llvm::cast_or_null<syntax::Leaf>(
-      findChild(syntax::NodeRole::CxxNullPtrExpression_keyword));
+      findChild(syntax::NodeRole::LiteralToken));
 }
 
 syntax::Expression *syntax::BinaryOperatorExpression::lhs() {
   return llvm::cast_or_null<syntax::Expression>(
       findChild(syntax::NodeRole::BinaryOperatorExpression_leftHandSide));
 }
 
 syntax::Leaf *syntax::UnaryOperatorExpression::operatorToken() {
   return llvm::cast_or_null<syntax::Leaf>(
-      findChild(syntax::NodeRole::UnaryOperatorExpression_operatorToken));
+      findChild(syntax::NodeRole::OperatorExpression_operatorToken));
 }
 
 syntax::Expression *syntax::UnaryOperatorExpression::operand() {
   return llvm::cast_or_null<syntax::Expression>(
       findChild(syntax::NodeRole::UnaryOperatorExpression_operand));
 }
 
 syntax::Leaf *syntax::BinaryOperatorExpression::operatorToken() {
   return llvm::cast_or_null<syntax::Leaf>(
-      findChild(syntax::NodeRole::BinaryOperatorExpression_operatorToken));
+      findChild(syntax::NodeRole::OperatorExpression_operatorToken));
 }
 
 syntax::Expression *syntax::BinaryOperatorExpression::rhs() {
   return llvm::cast_or_null<syntax::Expression>(
       findChild(syntax::NodeRole::BinaryOperatorExpression_rightHandSide));
 }
 
 syntax::Leaf *syntax::SwitchStatement::switchKeyword() {

 
 syntax::Leaf *syntax::ExplicitTemplateInstantiation::templateKeyword() {
   return llvm::cast_or_null<syntax::Leaf>(
       findChild(syntax::NodeRole::IntroducerKeyword));
 }
 
 syntax::Leaf *syntax::ExplicitTemplateInstantiation::externKeyword() {
   return llvm::cast_or_null<syntax::Leaf>(
-      findChild(syntax::NodeRole::ExplicitTemplateInstantiation_externKeyword));
+      findChild(syntax::NodeRole::ExternKeyword));
 }
 
 syntax::Declaration *syntax::ExplicitTemplateInstantiation::declaration() {
   return llvm::cast_or_null<syntax::Declaration>(
       findChild(syntax::NodeRole::ExplicitTemplateInstantiation_declaration));
 }
 
 syntax::Leaf *syntax::ParenDeclarator::lparen() {

 
 syntax::Leaf *syntax::ArraySubscript::rbracket() {
   return llvm::cast_or_null<syntax::Leaf>(
       findChild(syntax::NodeRole::CloseParen));
 }
 
 syntax::Leaf *syntax::TrailingReturnType::arrowToken() {
   return llvm::cast_or_null<syntax::Leaf>(
-      findChild(syntax::NodeRole::TrailingReturnType_arrow));
+      findChild(syntax::NodeRole::ArrowToken));
 }
 
 syntax::SimpleDeclarator *syntax::TrailingReturnType::declarator() {
   return llvm::cast_or_null<syntax::SimpleDeclarator>(
       findChild(syntax::NodeRole::TrailingReturnType_declarator));
 }
 
 syntax::Leaf *syntax::ParametersAndQualifiers::lparen() {