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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
