[PATCH] D24268: Traversing template paramter lists of DeclaratorDecls and/or TagDecls.
Łukasz Anforowicz via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 6 15:47:52 PDT 2016
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))))));
================
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
More information about the cfe-commits
mailing list