[PATCH] D24361: hasDeclaration(qualType(...)) matcher should unwrap ElaboratedType and TemplateSpecializationType

[Richard Smith via cfe-commits]

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 `Namespace::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).
> 
> WDYT?
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 case.


https://reviews.llvm.org/D24361