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

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/AST/RecursiveASTVisitor.h:487-489
@@ +486,5 @@
+  // Traverses template parameter lists of either a DeclaratorDecl or TagDecl.
+  template <typename T, typename Enabled = typename std::enable_if<
+      std::is_base_of<DeclaratorDecl, T>::value ||
+      std::is_base_of<TagDecl, T>::value>::type>
+  bool TraverseDeclTemplateParameterLists(T *D);
----------------
What's the purpose of the `enable_if` here?

================
Comment at: include/clang/AST/RecursiveASTVisitor.h:1728
@@ -1708,2 +1727,3 @@
 
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));
----------------
lukasza wrote:
> 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?
I don't think `ClassTemplateSpecializationDecl` can have a template parameter list for its qualifier. A `ClassTemplatePartialSpecializationDecl` can, though:

  template<typename T> struct A {
    template<typename U> struct B; 
  };
  template<typename T> template<typename U> struct A<T>::B<U*> {};

================
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()));
----------------
lukasza wrote:
> 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).
`VarTemplatePartialSpecializationDecl`s can have template parameter lists:

  template<typename T> struct A { template<typename U> static int n; };
  template<typename T> template<typename U> int A<T>::n<U*>;

I don't think other `VarTemplateSpecializationDecl`s can.

================
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()));
----------------
lukasza wrote:
> 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.
I'm happy to leave the refactoring (and its changes to traversal order / fixing missing traversal of type) to another day.

================
Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:1730
@@ +1729,3 @@
+  return type && InnerMatcher.matches(*type, Finder, Builder);
+}
+
----------------
lukasza wrote:
> 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?)
I think this is poorly named -- "underlying type" means something specific to do with enumerations, which is not what you're matching here. I'm also not sure why you need this at all; why can't you just match a `templateTypeParmType` directly?

================
Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:1741
@@ +1740,3 @@
+      qualType(hasUnderlyingType(templateTypeParmType(hasDeclaration(decl(
+          hasAncestor(decl()))))))));
+}
----------------
This seems like a non-obvious way to check that you matched the right `TemplateTypeParmDecl`. Can you check the source location of the match instead, perhaps?


https://reviews.llvm.org/D24268