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

[Richard Smith via cfe-commits]

On Tue, Sep 6, 2016 at 3:47 PM, Łukasz Anforowicz <lukasza at chromium.org>
wrote:

> lukasza 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);
> ----------------
> rsmith wrote:
> > What's the purpose of the `enable_if` here?
> enable_if is here to document via code that this method works for
> arguments that are either a TagDecl or a DeclaratorDecl.  enable_if is not
> really necessary here - I could take it out and just make the
> TraverseDeclTemplateParameterLists<T> method compile as long as T
> contains the right getNumTemplateParameterLists and
> getTemplateParameterList methods.
>
> FWIW, I got rid of std::enable_if above in the latest patch revision.
> Does the code look better and less surprising now?
>
> ================
> Comment at: include/clang/AST/RecursiveASTVisitor.h:1728
> @@ -1708,2 +1727,3 @@
>
> +  TRY_TO(TraverseDeclTemplateParameterLists(D));
>    TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));
> ----------------
> rsmith wrote:
> > 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*> {};
> Thanks for the example.  I've added a unit test covering this case (and
> verified that it fails before the fix and passes afterwards).  A bit
> surprisingly, the extra test passed without further tweaks to the product
> code (surprisingly, because I am not sure how TraverseDeclTemplateParameterLists
> ends up getting called for ClassTemplatePartialSpecializationDecl).  I
> don't think I want to keep digging to understand why things work without
> further tweaks, but the surprise factor makes me think the extra test is
> worth keeping (even though it apparently doesn't really increase test
> coverage).
>
> ================
> 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()));
> ----------------
> rsmith wrote:
> > 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.
> Thanks for the example.  I've added a unit test covering this case (and
> verified that it fails before the fix and passes afterwards).  A bit
> surprisingly, the extra test passed without further tweaks to the product
> code (surprisingly, because I am not sure how TraverseDeclTemplateParameterLists
> ends up getting called for VarTemplatePartialSpecializationDecl).  I
> don't think I want to keep digging to understand why things work without
> further tweaks, but the surprise factor makes me think the extra test is
> worth keeping (even though it apparently doesn't really increase test
> coverage).
>
> Also, I've found out that in my little write-up above I missed the
> following types of nodes derived from DeclaratorDecl (although looking at
> the code everything seems okay):
> - ObjCAtDefsFieldDecl - traversal calls TraverseDeclaratorHelper
> - ObjCIvarDecl - traversal calls TraverseDeclaratorHelper
> - MSPropertyDecl - traversal calls TraverseDeclaratorHelper
> - NonTypeTemplateParmDecl - traversal calls TraverseDeclaratorHelper
>
> ================
> 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()));
> ----------------
> rsmith wrote:
> > 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.
> Ack.
>
> ================
> Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:1730
> @@ +1729,3 @@
> +  return type && InnerMatcher.matches(*type, Finder, Builder);
> +}
> +
> ----------------
> rsmith wrote:
> > 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?
> Good point on the name - does hasBaseType sound better?  (not important
> for this patch, because as you point out I don't need this custom matcher,
> but possibly still important for the non-LLVM patch at
> https://codereview.chromium.org/2256913002).
>
> For the unit tests, I can indeed just use templateTypeParmType directly -
> done in the next patch revision.
>
> For https://codereview.chromium.org/2256913002, I think that I still need
> hasBaseType to only match DependentScopeDeclRefExpr if |T| in |T::foo|
> comes from a namespace I am interested in (e.g. T has to come from a
> namespace where my tool has to do massive renaming).  The code I used (and
> am not very happy about) in https://codereview.chromium.
> org/2256913002/patch/140001/150001 is:
>   auto in_blink_namespace = decl(
>       anyOf(decl_under_blink_namespace, decl_has_qualifier_to_blink_
> namespace,
>             hasAncestor(decl_has_qualifier_to_blink_namespace)),
>       unless(isExpansionInFileMatching(kGeneratedFileRegex)));
> ...
> +  auto blink_qual_type_matcher = qualType(
> +      anyOf(pointsTo(in_blink_namespace), references(in_blink_namespace),
> +            hasUnderlyingType(anyOf(
> +                enumType(hasDeclaration(in_blink_namespace)),
> +                recordType(hasDeclaration(in_blink_namespace)),
> +                templateSpecializationType(hasDeclaration(in_blink_
> namespace)),
> +                templateTypeParmType(hasDeclaration(in_blink_namespace)),
> +                typedefType(hasDeclaration(in_blink_namespace))))));
>

The hasBaseExprMissingOrMatching and hasQualifierMissingOrMatching matchers
in that patch seem reasonable to me, but I don't understand the purpose of
hasUnderlyingType -- it appears to convert a Type matcher into a QualType
matcher, but I thought that happened implicitly.

================
> Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:1741
> @@ +1740,3 @@
> +      qualType(hasUnderlyingType(templateTypeParmType(
> hasDeclaration(decl(
> +          hasAncestor(decl()))))))));
> +}
> ----------------
> rsmith wrote:
> > 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?
> I don't understand this feedback :-(
>
> If you are referring to the usage of |hasAncestor|, then this is in here
> to trigger the assert that fails the tests before the fix -
> assert(!Parents.empty() && "Found node that is not in the parent map.");
>
> I guess something like this would be an alternative unit test - before the
> fix this would fail, but instead of hitting the assert, it would fail to
> find a match.  In other words - this text expectation ensures the right
> match:
>   EXPECT_TRUE(matches(
>       "...",
>       templateTypeParmDecl(hasName("U"))));
> And this test expectation (more-or-less what I had in the originally
> proposed patch) catches the assert failure:
>   EXPECT_TRUE(matches(
>       "...",
>       templateTypeParmType(hasDeclaration(decl(hasAncestor(decl()))))));
>
> Are you saying that I should use only the second test expectation (the one
> with hasName)?  Both (i.e. the one with hasAncestor and the one with
> hasName)?  Something else?  FWIW, I switched to the |hasName| expectation
> in the most recent patch revision.
>
> PS. If we were to remove |hasAncestor| from the matcher used in the test
> cases, then I guess the test(suite) name wouldn't quite fit - this makes me
> think that I should rename the testcases from TEST(HasAncestor,
> TemplateTypeParmDeclUnder...) to TEST(TemplateTypeParmDecl, Under...).
>
>
> https://reviews.llvm.org/D24268
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160906/cdfa38b4/attachment-0001.html>