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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
