[PATCH] D24268: Traversing template paramter lists of DeclaratorDecls and/or TagDecls.

[Łukasz Anforowicz via cfe-commits]

lukasza added inline comments.

================
Comment at: include/clang/AST/RecursiveASTVisitor.h:1728
@@ -1708,2 +1727,3 @@
 
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));
----------------
I think that tweaks of EnumDecl traversal together with tweaks of TraverseRecordHelper tweaks, take care of all the relevant node types derived from TagDecl:

- EnumDecl - traversal now calls TraverseDeclTemplateParameterLists (+EnumDecl is covered by the new unit tests)
- RecordDecl - TraverseRecordHelper now calls TraverseDeclTemplateParameterLists (+RecordDecl is covered by the new unit tests)
- CXXRecordDecl - traversal includes a call to TraverseRecordHelper

I believe that nodes derived further from CXXRecordDecl cannot have a template parameter list (?):
- ClassTemplateSpecializationDecl
- ClassTemplatePartialSpecializationDecl
If this is not correct, then can you please help me see the shape of unit tests that would cover these nodes?

================
Comment at: include/clang/AST/RecursiveASTVisitor.h:1823
@@ -1802,2 +1822,3 @@
 bool RecursiveASTVisitor<Derived>::TraverseDeclaratorHelper(DeclaratorDecl *D) {
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));
----------------
I think that TraverseDeclaratorHelper together with TraverseFunctionHelper tweaks, this takes care of all node types derived from DeclaratorDecl:
- FieldDecl - traversal calls TraverseDeclaratorHelper
- FunctionDecl - traversal calls TraverseFunctionHelper
- CXXMethodDecl - traversal calls TraverseFunctionHelper (+CXXMethodDecl is covered by the new unit tests)
- CXXConstructorDecl - traversal calls TraverseFunctionHelper
- CXXConversionDecl - traversal calls TraverseFunctionHelper
- CXXDestructorDecl - traversal calls TraverseFunctionHelper
- VarDecl - TraverseVarHelper calls TraverseDeclaratorHelper (+VarDecl is covered by the new unit tests)

I believe that nodes derived further from VarDecl cannot have a template parameter list (?), but most of them should still be safe (because their traversal goes through TraverseVarHelper):
- DecompositionDecl - traversal calls TraverseVarHelper
- ImplicitParamDecl - traversal calls TraverseVarHelper
- OMPCapturedExprDecl - traversal calls TraverseVarHelper
- ParmVarDecl - traversal calls TraverseVarHelper
- VarTemplateSpecializationDecl
- VarTemplatePartialSpecializationDecl

Please let me know if I am wrong above and it is indeed possible to have template parameter lists next to VarTemplateSpecializationDecl and/or VarTemplatePartialSpecializationDecl (and I would also appreciate if you could help me see the shape of unit tests that would cover these nodes).

================
Comment at: include/clang/AST/RecursiveASTVisitor.h:1870
@@ -1848,2 +1869,3 @@
 bool RecursiveASTVisitor<Derived>::TraverseFunctionHelper(FunctionDecl *D) {
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));
----------------
Richard, in https://llvm.org/bugs/show_bug.cgi?id=29042#c6 you've suggested reusing TraverseDeclaratorHelper here.  I agree that there are refactoring and code reusing opportunities here (e.g. TraverseNestedNameSpecifier[Loc] are getting called from both TraverseDeclaratorHelper and from TraverseFunctionHelper), but I didn't pursue them in this patch, because I was worried about little details:

1. traversal order (i.e. TraverseDeclaratorHelper traverses NestedNameSpecifier immediately before traversing TypeSourceInfo;  this is different than TraverseFunctionHelper which traverses DeclarationNameInfo in-between traversing NestedNameSpecifier and TypeSourceInfo)

2. traversal in absence of TypeSourceInfo (i.e. when TypeSourceInfo is missing, then TraverseDeclaratorHelper traverses Type;  this is different than TraverseFunctionHelper which in this case 1) checks shouldVisitImplicitCode condition and only then 2) traverses function parameters).

Because of the above, I think that for now it is better to call TraverseDeclTemplateParameterLists directly from TraverseFunctionHelper, rather than trying to reuse the whole (or bigger part of) TraverseDeclaratorHelper from TraverseFunctionHelper.

================
Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:1730
@@ +1729,3 @@
+  return type && InnerMatcher.matches(*type, Finder, Builder);
+}
+
----------------
I've needed this custom AST matcher in https://codereview.chromium.org/2256913002 - let me try to sneak in a question related to this other CL:  Can I somehow avoid introducing a new, custom AST matcher?  (i.e. is it possible to express hasUnderlyingType in terms of built-in matchers?)


https://reviews.llvm.org/D24268