lukasza added inline comments.
================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:750
@@ +749,3 @@
+ else if (auto *ET = Node->getAs<ElaboratedType>())
+ return matchesSpecialized(ET->getNamedType(), Finder, Builder);
+ else if (auto *TST = Node->getAs<TemplateSpecializationType>())
----------------
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 &Node,
> + ASTMatchFinder *Finder,
> + BoundNodesTreeBuilder *Builder) const {
> + return matchesSpecialized(Node.getNamedType(), Finder, Builder);
> + }
> +
> ...
> /// \brief All types that are supported by HasDeclarationMatcher above.
> -typedef TypeList<CallExpr, CXXConstructExpr, DeclRefExpr, EnumType,
> - InjectedClassNameType, LabelStmt, AddrLabelExpr,
> MemberExpr,
> - QualType, RecordType, TagType,
> TemplateSpecializationType,
> - TemplateTypeParmType, TypedefType,
> +typedef TypeList<AutoType, CallExpr, CXXConstructExpr, DeclRefExpr,
> 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<TemplateSpecializationType>())
+ return matchesSpecialized(*TST, Finder, Builder);
else if (auto *TT = Node->getAs<TypedefType>())
----------------
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<typename T>
class Foo {};
template <typename T>
using Bar = Foo<T>;
Bar<int> 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 <typename U>\n"
+ "void Function(Namespace::Template<U> param) {\n"
+ " param.Method();\n"
----------------
rsmith wrote:
> @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)).
While waiting for klimek@'s reply, let me share my perspective.
hasDeclaration matching parmVarDecl has come up for me when working on a
tool that needs to rename all methods in a given namespace from
camelCase to PascalCase. If the tool can't see that a parmVarDecl's
declaration is from the targeted namespace, then it will fail to rename
methods invoked on that parameter.
Unfortunately, I lost track of a real repro in the wild (when trying to
run the renaming tool on Chromium), so I can only cook-up an artificial
example below. Hopefully the example is useful for illustrating what I
am talking about above:
namespace NamespaceWhereMethodsShouldBeRenamed {
template <typename T>
class Template {
public:
// All methods in NamespaceWhereMethodsShouldBeRenamed
// will be renamed from camelCase to PascalCase - i.e.
// |fooBar| below will be renamed to |FooBar|.
void fooBar() {}
};
}
namespace SomeOtherNamespace {
class OtherClass {
public:
void barBaz() {}
}
}
template <typename U>
void Function(::NamespaceWhereMethodsShouldBeRenamed::Template<U>* p) {
// |fooBar| should be rewritten to |FooBar|, but this will only
// happen if we can see that |p|'s declaration comes from
// NamespaceWhereMethodsShouldBeRenamed - we won't be able to see
// that if we claim that |p| has no declaration.
p->fooBar();
// Obviously |barBaz| should not be rewritten below, because |x|
// doesn't come from NamespaceWhereMethodsShouldBeRenamed
// namespace.
::SomeOtherNamespace::OtherClass x;
x.barBaz():
}
So - answer #1 is undesirable for me (because it would make my tool miss
the method renaming - i.e. it wouldn't rename |fooBar| to |FooBar| in
the example above). I don't really care whether the answer is #2
(classTemplateDecl) vs #3 (cxxRecordDecl) since both are in the same
namespace. FWIW, my naive / simple fix results in answer #2 (which I've
reflected in the unit tests in the latest patch proposal).
BTW: After reading the earlier comment about *step-by-step* desugaring,
I wonder whether the answer should be that *both* #2 and #3 should match.
I also wonder if this change should be done in a separate CL (because it
doesn't seem related to ElaboratedType and TemplateSpecializationType
cases I've added in the patches proposed here + I am a bit uncertain to
what cases exactly such step-by-step decomposing should be added).
================
Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2130
@@ +2129,3 @@
+ // hasDeclaration / qualType-flavour.
+ EXPECT_TRUE(matches(input, parmVarDecl(
+ hasName("param"),
----------------
Done.
https://reviews.llvm.org/D24361
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
