rsmith added inline comments.

Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:752
@@ -749,1 +751,3 @@
+    else if (auto *TST = Node->getAs<TemplateSpecializationType>())
+      return matchesSpecialized(*TST, Finder, Builder);
     else if (auto *TT = Node->getAs<TypedefType>())
lukasza wrote:
> I am a little bit confused and concerned whether the order of the if 
> statements here matters.  In particular - I am not sure if the ordering of 
> dyn_cast below and getAs calls here matters (and whether things would be 
> safer / less fragile if the dyn_cast was moved all the way to the top?).
I don't think the position of the `TemplateTypeParmType` case matters; a 
non-canonical TTPT always desugars to a canonical TTPT, so none of the other 
cases can match.

The relative order of the `getAs` cases can matter, though, and neither 
ordering of `TypedefType` and `TemplateSpecializationType` will actually do the 
right thing in general (you can have a typedef that desugars to a TST and vice 
versa -- the TST would need to represent an alias template specialization in 
the latter case). If you want to get this "right" you'll need to do single-step 
desugaring until you hit a type you like the look of. (See `getAsSugar` in 
lib/AST/Type.cpp for an example.)

Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2119
@@ +2118,3 @@
+      "template <typename U>\n"
+      "void Function(Namespace::Template<U> param) {\n"
+      "  param.Method();\n"
@klimek, would you expect `hasDeclaration` to match the type of `param` here, 
and if so, what should it produce? There seem to be three possible answers:

1) There is a declaration for `Namespace::Template`, but not for the type 
`Namespace::Template<`parameter 0 of `Function>`, so no declaration is matched.

2) This matches and produces the declaration of the template 

3) This matches and produces the declaration of the class *within* the 
declaration of that template.

(The difference between 2 and 3 is whether you produce a `CXXRecordDecl` or a 
`ClassTemplateDecl`.) WDYT? The existing behavior for `matchesSpecialized` on a 
`TemplateSpecializationType` makes me think that (2) is expected (but I'd have 
personally expected (3)).

Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2130
@@ +2129,3 @@
+      hasName("param"),
+      hasType(qualType(hasDeclaration(decl(anything())))))));
lukasza wrote:
> I was a little bit torn between using |anything()| above (i.e. testing that a 
> parameter type in this test should always have _a_ corresponding declaration) 
> VS using |templateDecl()| instead (i.e. having a more specific verification / 
> testing that a parameter type here has _the_ right corresponding declaration).
I think we should be testing that we get the right declaration here, especially 
since (as noted above) there are actually two reasonable possibilities in this 

cfe-commits mailing list

Reply via email to