Re: [PATCH] D24361: hasDeclaration(qualType(...)) matcher should unwrap ElaboratedType and TemplateSpecializationType
klimek added inline comments.
Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2119
@@ +2118,3 @@
+ "template \n"
+ "void Function(Namespace::Template param) {\n"
+ " param.Method();\n"
Given your use case: why do we need hasDeclaration here at all?
I'd have expected this working with just matching on the nested name specifier
of the type instead of saying hasDeclaration on the template type.
Btw, if you add a type alias for a class not in the namespace into the
namespace (typedef / using), do you wan that to rename or not? :)
I'd personally probably have expected (2), but I'm never sure in these cases
without playing around with more test cases...
https://reviews.llvm.org/D24361
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24361: hasDeclaration(qualType(...)) matcher should unwrap ElaboratedType and TemplateSpecializationType
lukasza updated the summary for this revision.
lukasza updated this revision to Diff 72350.
lukasza marked an inline comment as done.
lukasza added a comment.
- Added test where both TemplateSpecializationType and TypedefType are present
and both should match regardless of code order inside
HasDeclarationMatcher::matchesSpecialized(const QualType ). Removed
significance of order inside this matchesSpecialized method.
- Tweaked test proposed in the previous patchset, so that it matches specific
decls (rather than any decls)
- Added support for matching hasDeclaration(elaboratedType(...)) + test (in the
test from the previous patchset).
https://reviews.llvm.org/D24361
Files:
include/clang/ASTMatchers/ASTMatchers.h
include/clang/ASTMatchers/ASTMatchersInternal.h
unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
Index: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2106,5 +2106,56 @@
functionDecl(hasName("bar"));
}
+TEST(HasDeclaration, ElaboratedTypeAndTemplateSpecializationType) {
+ std::string input =
+ "namespace Namespace {\n"
+ "template\n"
+ "class Template {\n"
+ " public:\n"
+ " void Method() {}\n"
+ "};\n"
+ "} // namespace Namespace\n"
+ "template \n"
+ "void Function(Namespace::Template param) {\n"
+ " param.Method();\n"
+ "};\n";
+
+ // Matcher for ::Namespace::Template template decl.
+ auto param_type_decl_matcher = classTemplateDecl(
+ hasName("Template"),
+ hasParent(namespaceDecl(hasName("Namespace"))),
+ has(templateTypeParmDecl(hasName("T";
+
+ // hasDeclaration / qualType-flavour.
+ EXPECT_TRUE(matches(input, parmVarDecl(
+ hasName("param"),
+ hasType(qualType(hasDeclaration(decl(param_type_decl_matcher)));
+
+ // hasDeclaration / elaboratedType-flavour.
+ EXPECT_TRUE(matches(input, parmVarDecl(
+ hasName("param"),
+ hasType(elaboratedType(hasDeclaration(decl(param_type_decl_matcher)));
+}
+
+TEST(HasDeclaration, TypedefTypeAndTemplateSpecializationTypeAreBothHere) {
+ std::string input =
+ "template\n"
+ "class Foo {};\n"
+ "using Bar = Foo;\n"
+ "Bar variable;\n";
+
+ // hasDeclaration should match typedefNameDecl.
+ EXPECT_TRUE(matches(input, varDecl(
+ hasName("variable"),
+ hasType(qualType(hasDeclaration(decl(
+ typeAliasDecl(hasName("Bar");
+
+ // hasDeclaration should match templateSpecializationType
+ EXPECT_TRUE(matches(input, varDecl(
+ hasName("variable"),
+ hasType(qualType(hasDeclaration(decl(
+ classTemplateSpecializationDecl(;
+}
+
} // namespace ast_matchers
} // namespace clang
Index: include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- include/clang/ASTMatchers/ASTMatchersInternal.h
+++ include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -744,16 +744,29 @@
if (Node.isNull())
return false;
-if (auto *TD = Node->getAsTagDecl())
- return matchesDecl(TD, Finder, Builder);
-else if (auto *TT = Node->getAs())
- return matchesDecl(TT->getDecl(), Finder, Builder);
// Do not use getAs instead of the direct dyn_cast.
// Calling getAs will return the canonical type, but that type does not
// store a TemplateTypeParmDecl. We *need* the uncanonical type, if it is
// available, and using dyn_cast ensures that.
-else if (auto *TTP = dyn_cast(Node.getTypePtr()))
+if (auto *TTP = dyn_cast(Node.getTypePtr()))
return matchesDecl(TTP->getDecl(), Finder, Builder);
+
+// Try if any of desugaring results match
+// (these are not mutually exclusive).
+if (auto *TST = Node->getAs()) {
+ if (matchesSpecialized(*TST, Finder, Builder))
+return true;
+}
+if (auto *TT = Node->getAs()) {
+ if (matchesDecl(TT->getDecl(), Finder, Builder))
+return true;
+}
+
+// Try non-desugarable types.
+if (auto *TD = Node->getAsTagDecl())
+ return matchesDecl(TD, Finder, Builder);
+else if (auto *ET = Node->getAs())
+ return matchesSpecialized(*ET, Finder, Builder);
else if (auto *OCIT = Node->getAs())
return matchesDecl(OCIT->getDecl(), Finder, Builder);
else if (auto *UUT = Node->getAs())
@@ -760,9 +773,18 @@
return matchesDecl(UUT->getDecl(), Finder, Builder);
else if (auto *ICNT = Node->getAs())
return matchesDecl(ICNT->getDecl(), Finder, Builder);
+
return false;
}
+ /// \brief Gets the QualType from ElaboratedType
+ /// and returns whether the inner matches on it.
+ bool matchesSpecialized(const ElaboratedType ,
+ ASTMatchFinder *Finder,
+ BoundNodesTreeBuilder *Builder)
Re: [PATCH] D24361: hasDeclaration(qualType(...)) matcher should unwrap ElaboratedType and TemplateSpecializationType
lukasza added inline comments.
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:750
@@ +749,3 @@
+else if (auto *ET = Node->getAs())
+ return matchesSpecialized(ET->getNamedType(), Finder, Builder);
+else if (auto *TST = Node->getAs())
lukasza wrote:
> There is also a question whether I should not only start supporting not only
> qualType(hasDeclaration(...)) when qualType wraps an ElaboratedType node, but
> also start supporting elaboratedType(hasDeclaration(...)).
>
> Snippets of a patch that could achieve this (but then again - I am not sure
> if this is desirable and/or necessary):
>
> +++ include/clang/ASTMatchers/ASTMatchersInternal.h (working copy)
> ...
> + /// \brief Gets the QualType from ElaboratedType
> + /// and returns whether the inner matches on it.
> + bool matchesSpecialized(const ElaboratedType ,
> + ASTMatchFinder *Finder,
> + BoundNodesTreeBuilder *Builder) const {
> +return matchesSpecialized(Node.getNamedType(), Finder, Builder);
> + }
> +
> ...
> /// \brief All types that are supported by HasDeclarationMatcher above.
> -typedef TypeList - InjectedClassNameType, LabelStmt, AddrLabelExpr,
> MemberExpr,
> - QualType, RecordType, TagType,
> TemplateSpecializationType,
> - TemplateTypeParmType, TypedefType,
> +typedef TypeList ElaboratedType,
> + EnumType, InjectedClassNameType, LabelStmt,
> AddrLabelExpr,
> + MemberExpr, QualType, RecordType, TagType,
> + TemplateSpecializationType, TemplateTypeParmType,
> TypedefType,
> UnresolvedUsingType> HasDeclarationSupportedTypes;
FWIW, I've added this support in the latest patchset.
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:752
@@ -749,1 +751,3 @@
+else if (auto *TST = Node->getAs())
+ return matchesSpecialized(*TST, Finder, Builder);
else if (auto *TT = Node->getAs())
rsmith wrote:
> 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.)
Thank you for the explanation and the hint to look at getAsSugar.
I've added a test where both TypeSpecializationType and TypedefType could be
used. One of the asserts (the one trying to match typeAliasDecl) was initially
failing as expected / as explained by your comment above (i.e.
HasDeclarationMatcher::matchesSpecialized(QualType...) was first looking at
TypeSpecializationType and when that succeeded it would not consider
TypedefType later on). The test covers
alias-desugars-to-template-specialization-type case.
Unfortunately, I was not able to come up with desugaring going in the other
direction. I started with something like what I have below, but there is no
typeAliasDecl here (I think) and I don't know how to insert one:
template
class Foo {};
template
using Bar = Foo;
Bar variable;
I am not sure if I really need to do single-step desugaring (and consult
hasDeclaration's innerMatcher for each step). Wouldn't it be sufficient that
desugaring to TemplateSpecializationType does not latch onto TST code path, but
that we will also consider TypedefType desugaring later? Code is probably
better at explaining at what I mean here - please look at the latest patchset.
The new test passes, so I think my changes really made ordering insignificant
as desired (at least insignificant when it comes to ordering between the only
types (TemplateSpecializationType and TypedefType) that are 1) used in
HasDeclarationMatcher / QualType overload + 2) covered via getAsSugar helper).
Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2119
@@ +2118,3 @@
+ "template \n"
+ "void
Re: [PATCH] D24361: hasDeclaration(qualType(...)) matcher should unwrap ElaboratedType and TemplateSpecializationType
lukasza added a comment.
Richard, could you please take a look?
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:750
@@ +749,3 @@
+else if (auto *ET = Node->getAs())
+ return matchesSpecialized(ET->getNamedType(), Finder, Builder);
+else if (auto *TST = Node->getAs())
There is also a question whether I should not only start supporting not only
qualType(hasDeclaration(...)) when qualType wraps an ElaboratedType node, but
also start supporting elaboratedType(hasDeclaration(...)).
Snippets of a patch that could achieve this (but then again - I am not sure if
this is desirable and/or necessary):
+++ include/clang/ASTMatchers/ASTMatchersInternal.h (working copy)
...
+ /// \brief Gets the QualType from ElaboratedType
+ /// and returns whether the inner matches on it.
+ bool matchesSpecialized(const ElaboratedType ,
+ ASTMatchFinder *Finder,
+ BoundNodesTreeBuilder *Builder) const {
+return matchesSpecialized(Node.getNamedType(), Finder, Builder);
+ }
+
...
/// \brief All types that are supported by HasDeclarationMatcher above.
-typedef TypeList HasDeclarationSupportedTypes;
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:753
@@ -749,2 +752,3 @@
+ return matchesSpecialized(*TST, Finder, Builder);
else if (auto *TT = Node->getAs())
return matchesDecl(TT->getDecl(), Finder, Builder);
I do notice that there are quite a few similar Node->getAs if statements
here. They could be replaced with something "clever" (and probably unreadable):
+++ include/clang/ASTMatchers/ASTMatchersInternal.h (working copy)
...
+ /// \brief Terminating case for recursion over template parameters
+ /// performed by matchesQualTypeAsAnyOf template.
+ template
+ bool matchesQualTypeAsAnyOf(const QualType , ASTMatchFinder *Finder,
+ BoundNodesTreeBuilder *Builder) const {
+return false;
+ }
+
+ /// \brief Returns true if Node->getAs is not null and matches inner
+ /// matcher for any type U listed in template parameters.
+ template ::value, int>::type = 0>
+ bool matchesQualTypeAsAnyOf(const QualType , ASTMatchFinder *Finder,
+ BoundNodesTreeBuilder *Builder) const {
+if (const U *u = Node->getAs())
+ return matchesSpecialized(*u, Finder, Builder);
+
+return matchesQualTypeAsAnyOf(Node, Finder, Builder);
+ }
+
...
-else if (auto *OCIT = Node->getAs())
- return matchesDecl(OCIT->getDecl(), Finder, Builder);
-else if (auto *UUT = Node->getAs())
- return matchesDecl(UUT->getDecl(), Finder, Builder);
-else if (auto *ICNT = Node->getAs())
- return matchesDecl(ICNT->getDecl(), Finder, Builder);
-return false;
+else if (auto *TD = Node->getAsTagDecl())
+ return matchesDecl(TD, Finder, Builder);
+
+return matchesQualTypeAsAnyOf<
+ArrayType, InjectedClassNameType, ObjCInterfaceType,
TemplateSpecializationType,
+TypedefType, UnresolvedUsingType, void>(
+Node, Finder, Builder);
Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2124
@@ +2123,3 @@
+ // hasDeclaration should see through:
+ // 1. from ElaboratedType (Namespace::MyTemplate) to
+ //TemplateSpecializationType (MyTemplate)
FWIW, I couldn't repro a similar issue (i.e. VarDecl or ParmVarDecl of a type
with user-provided declaration, but where hasDeclaration doesn't match
*anything*) for other types (i.e. AutoType or ArrayType) - only for
ElaboratedType and TemplateSpecializationType.
https://reviews.llvm.org/D24361
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24361: hasDeclaration(qualType(...)) matcher should unwrap ElaboratedType and TemplateSpecializationType
rsmith added inline comments.
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:752
@@ -749,1 +751,3 @@
+else if (auto *TST = Node->getAs())
+ return matchesSpecialized(*TST, Finder, Builder);
else if (auto *TT = Node->getAs())
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 \n"
+ "void Function(Namespace::Template 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
