[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

393

I would argue that this is much clearer :)

1465

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

207

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. ....

225–226

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

[gribozavr2]

clang/lib/Tooling/Syntax/BuildTree.cpp

202

207

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

207

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_None:
     llvm_unreachable("Not an overloadable operator");
   }
   llvm_unreachable("Unknown OverloadedOperatorKind enum");
 }
 
 /// 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+`.
 static SourceRange getDeclaratorRange(const SourceManager &SM, TypeLoc T,
                                       SourceLocation Name,
                                       SourceRange Initializer) {
   SourceLocation Start = GetStartLoc().Visit(T);
   SourceLocation End = T.getSourceRange().getEnd();
   assert(End.isValid());
   if (Name.isValid()) {
     if (Start.isInvalid())
       Start = Name;
     if (SM.isBeforeInTranslationUnit(End, Name))
       End = Name;
   }
   if (Initializer.isValid()) {
     auto InitializerEnd = Initializer.getEnd();
     assert(SM.isBeforeInTranslationUnit(End, InitializerEnd) ||
            End == InitializerEnd);
     End = InitializerEnd;
   }
   return SourceRange(Start, End);
 }
 
 namespace {
 /// All AST hierarchy roots that can be represented as pointers.
 using ASTPtr = llvm::PointerUnion<Stmt *, Decl *>;
 /// Maintains a mapping from AST to syntax tree nodes. This class will get more
 /// complicated as we support more kinds of AST nodes, e.g. TypeLocs.
 /// FIXME: expose this as public API.
 class ASTToSyntaxMapping {
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.");
+  SourceLocation getQualifiedNameStart(NamedDecl *D) {
+    assert((isa<DeclaratorDecl, TypedefNameDecl>(D)) &&
 
     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()) {
SourceRange getInitializerRange(Decl *D) {
     }
 
     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);
     }
private:
   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());
     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.