[SyntaxTree][NFC] Refactor function templates into functions taking base class

[eduucaldas]

Summary

The refactored functions were

• isReponsibleForCreatingDeclaration
• getQualifiedNameStart

[eduucaldas]

Fix macro

[eduucaldas]

Inline getDeclaratorRange inside buildTrailingReturn

[eduucaldas]

As a result of this change getDeclaratorRange is used exclusively inside processDeclaratorAndDeclaration and the last two arguments are direct results of getQualifiedNameStart(D) and getInitializerRange(D), which are used exclusively in this context.

Perhaps we should inline getQualifiedNameStart and getInitializerRange and make getDeclaratorRange a member function taking a Decl. What do you think?

clang/lib/Tooling/Syntax/BuildTree.cpp

432

I would argue that this is much clearer :)

1472–1473

1. TrailingReturn is not a Declarator
2. Inlining the arguments is actually less code and much than using the getDeclaratorRange here.

[gribozavr2]

Perhaps we should inline getQualifiedNameStart and getInitializerRange and make getDeclaratorRange a member function taking a Decl. What do you think?

These helpers seem to have well-defined semantics, so I'd rather keep them separate. If anything, I'd make helpers into free functions to emphasize that they don't use the class member variables and are standalone functions.

[eduucaldas]

Move getDeclaratorRange helpers outside of TreeBuilder
Add coverage for qualified declarators and init-declarators

[eduucaldas]

clang/lib/Tooling/Syntax/BuildTree.cpp

225–226

I followed the grammar to get these and also added tests for them.

245

There was no problem with the declarator of operator+ should I remove this?
Also I really tried to break it ^^.

I think though perhaps this is going to bite us in an obscure way, so I would just remove the e.g. ....

[gribozavr2]

clang/lib/Tooling/Syntax/BuildTree.cpp

202

245

Feel free to remove the fixme.

What do you mean by "remove the e.g. ..."? Removing the examples from the comment? Why?

[eduucaldas]

Rebase and answer comments

[eduucaldas]

clang/lib/Tooling/Syntax/BuildTree.cpp

245

I meant

/// FIXME: \p Name must be a source range, e.g. for `operator+`.

->

/// FIXME: \p Name must be a source range.

clang/lib/Tooling/Syntax/BuildTree.cpp

static syntax::NodeKind getOperatorNodeKind(const CXXOperatorCallExpr &E) {
   case OO_Conditional: // not overloadable
   case NUM_OVERLOADED_OPERATORS:
   case OO_None:
     llvm_unreachable("Not an overloadable operator");
   }
   llvm_unreachable("Unknown OverloadedOperatorKind enum");
 }
 
+/// Get the start of the qualified name. In the examples below it gives the
+/// location of the `^`:
+///     `int ^a;`
+///     `int ^a::S::f(){}`
+static SourceLocation getQualifiedNameStart(NamedDecl *D) {
+  assert((isa<DeclaratorDecl, TypedefNameDecl>(D)) &&
+         "only DeclaratorDecl and TypedefNameDecl are supported.");
+
+  auto DN = D->getDeclName();
+  bool IsAnonymous = DN.isIdentifier() && !DN.getAsIdentifierInfo();
+  if (IsAnonymous)
+    return SourceLocation();
+
+  if (const auto *DD = dyn_cast<DeclaratorDecl>(D)) {
+    if (DD->getQualifierLoc()) {
+      return DD->getQualifierLoc().getBeginLoc();
+    }
+  }
+
+  return D->getLocation();
+}
+
+/// Gets the range of the initializer inside an init-declarator C++ [dcl.decl].
+///     `int a;` -> range of ``,
+///     `int *a = nullptr` -> range of `= nullptr`.
+///     `int a{}` -> range of `{}`.
+///     `int a()` -> range of `()`.
+static SourceRange getInitializerRange(Decl *D) {
+  if (auto *V = dyn_cast<VarDecl>(D)) {
+    auto *I = V->getInit();
+    // Initializers in range-based-for are not part of the declarator
+    if (I && !V->isCXXForRangeDecl())
+      return I->getSourceRange();
+  }
+
+  return SourceRange();
+}
+
 /// Gets the range of declarator as defined by the C++ grammar. E.g.
 ///     `int a;` -> range of `a`,
 ///     `int *a;` -> range of `*a`,
 ///     `int a[10];` -> range of `a[10]`,
 ///     `int a[1][2][3];` -> range of `a[1][2][3]`,
 ///     `int *a = nullptr` -> range of `*a = nullptr`.
-/// FIMXE: \p Name must be a source range, e.g. for `operator+`.
+///     `int S::f(){}` -> range of `S::f()`.
+/// FIXME: \p Name must be a source range, e.g. for `operator+`.
 static SourceRange getDeclaratorRange(const SourceManager &SM, TypeLoc T,
                                       SourceLocation Name,
                                       SourceRange Initializer) {
   SourceLocation Start = GetStartLoc().Visit(T);
-  SourceLocation End = T.getSourceRange().getEnd();
+  SourceLocation End = T.getEndLoc();
   assert(End.isValid());
   if (Name.isValid()) {
     if (Start.isInvalid())
       Start = Name;
     if (SM.isBeforeInTranslationUnit(End, Name))
       End = Name;
   }
   if (Initializer.isValid()) {
public:
   ArrayRef<syntax::Token>
   getTemplateRange(const ClassTemplateSpecializationDecl *D) const {
     auto Tokens = getRange(D->getSourceRange());
     return maybeAppendSemicolon(Tokens, D);
   }
 
   /// Returns true if \p D is the last declarator in a chain and is thus
   /// reponsible for creating SimpleDeclaration for the whole chain.
-  template <class T>
-  bool isResponsibleForCreatingDeclaration(const T *D) const {
-    static_assert((std::is_base_of<DeclaratorDecl, T>::value ||
-                   std::is_base_of<TypedefNameDecl, T>::value),
-                  "only DeclaratorDecl and TypedefNameDecl are supported.");
+  bool isResponsibleForCreatingDeclaration(const Decl *D) const {
+    assert((isa<DeclaratorDecl, TypedefNameDecl>(D)) &&
 
     const Decl *Next = D->getNextDeclInContext();
 
     // There's no next sibling, this one is responsible.
     if (Next == nullptr) {
       return true;
     }
-    const auto *NextT = dyn_cast<T>(Next);
 
     // Next sibling is not the same type, this one is responsible.
-    if (NextT == nullptr) {
+    if (D->getKind() != Next->getKind()) {
       return true;
     }
     // Next sibling doesn't begin at the same loc, it must be a different
     // declaration, so this declarator is responsible.
-    if (NextT->getBeginLoc() != D->getBeginLoc()) {
+    if (Next->getBeginLoc() != D->getBeginLoc()) {
       return true;
     }
 
     // NextT is a member of the same declaration, and we need the last member to
     // create declaration. This one is not responsible.
     return false;
   }
 
public:
 
   bool WalkUpFromTypeAliasDecl(TypeAliasDecl *S) {
     Builder.foldNode(Builder.getDeclarationRange(S),
                      new (allocator()) syntax::TypeAliasDeclaration, S);
     return true;
   }
 
 private:
-  template <class T> SourceLocation getQualifiedNameStart(T *D) {
-    static_assert((std::is_base_of<DeclaratorDecl, T>::value ||
-                   std::is_base_of<TypedefNameDecl, T>::value),
-                  "only DeclaratorDecl and TypedefNameDecl are supported.");
-
-    auto DN = D->getDeclName();
-    bool IsAnonymous = DN.isIdentifier() && !DN.getAsIdentifierInfo();
-    if (IsAnonymous)
-      return SourceLocation();
-
-    if (const auto *DD = dyn_cast<DeclaratorDecl>(D)) {
-      if (DD->getQualifierLoc()) {
-        return DD->getQualifierLoc().getBeginLoc();
-      }
-    }
-
-    return D->getLocation();
-  }
-
-  SourceRange getInitializerRange(Decl *D) {
-    if (auto *V = dyn_cast<VarDecl>(D)) {
-      auto *I = V->getInit();
-      // Initializers in range-based-for are not part of the declarator
-      if (I && !V->isCXXForRangeDecl())
-        return I->getSourceRange();
-    }
-
-    return SourceRange();
-  }
-
   /// Folds SimpleDeclarator node (if present) and in case this is the last
   /// declarator in the chain it also folds SimpleDeclaration node.
   template <class T> bool processDeclaratorAndDeclaration(T *D) {
-    SourceRange Initializer = getInitializerRange(D);
-    auto Range = getDeclaratorRange(Builder.sourceManager(),
-                                    D->getTypeSourceInfo()->getTypeLoc(),
-                                    getQualifiedNameStart(D), Initializer);
+    auto Range = getDeclaratorRange(
+        Builder.sourceManager(), D->getTypeSourceInfo()->getTypeLoc(),
+        getQualifiedNameStart(D), getInitializerRange(D));
 
     // There doesn't have to be a declarator (e.g. `void foo(int)` only has
     // declaration, but no declarator).
     if (Range.getBegin().isValid()) {
       auto *N = new (allocator()) syntax::SimpleDeclarator;
       Builder.foldNode(Builder.getRange(Range), N, nullptr);
       Builder.markChild(N, syntax::NodeRole::Declarator);
     }
 
     if (Builder.isResponsibleForCreatingDeclaration(D)) {
       Builder.foldNode(Builder.getDeclarationRange(D),
                        new (allocator()) syntax::SimpleDeclaration, D);
     }
     return true;
   }
 
   /// Returns the range of the built node.
   syntax::TrailingReturnType *buildTrailingReturn(FunctionProtoTypeLoc L) {
     assert(L.getTypePtr()->hasTrailingReturn());
 
     auto ReturnedType = L.getReturnLoc();
     // Build node for the declarator, if any.
-    auto ReturnDeclaratorRange =
-        getDeclaratorRange(this->Builder.sourceManager(), ReturnedType,
-                           /*Name=*/SourceLocation(),
-                           /*Initializer=*/SourceLocation());
+    auto ReturnDeclaratorRange = SourceRange(GetStartLoc().Visit(ReturnedType),
+                                             ReturnedType.getEndLoc());
     syntax::SimpleDeclarator *ReturnDeclarator = nullptr;
     if (ReturnDeclaratorRange.isValid()) {
       ReturnDeclarator = new (allocator()) syntax::SimpleDeclarator;
       Builder.foldNode(Builder.getRange(ReturnDeclaratorRange),
                        ReturnDeclarator, nullptr);
     }
 
     // Build node for trailing return type.