On Tue, Sep 6, 2016 at 3:47 PM, Łukasz Anforowicz <luka...@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
>
>
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
