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

[Manuel Klimek via cfe-commits]

klimek added inline comments.


================
Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2119
+      "template <typename U>\n"
+      "void Function(Namespace::Template<U> param) {\n"
+      "  param.Method();\n"
----------------
lukasza wrote:
> klimek wrote:
> > 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...
> > 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.
> 
> Because I want "namespace-of-user-provided-declaration" matching to work both for ElaboratedType nodes (with explicit nested name specifier) and for other kinds of nodes (where there might be no nested name specifier).  I was hoping that I could do this with a single hasDeclaration matcher, rather than listing all possible type nodes myself (when building my own matcher) like I sort of do in a workaround.  In particular, after this CL a single, simple hasDeclaration-based matcher can be used in
>     //    auto blink_qual_type_base_matcher =
>     //        qualType(hasDeclaration(in_blink_namespace));
> inside https://codereview.chromium.org/2256913002/patch/180001/190001.
> 
> > 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? :)
> 
> Good question.  I want a rename to happen if I have ::SomeOtherNamespace::Typedef resolving to ::NamespaceWithRenamedMethods::Class, but I do not want rename to happen if I have ::NamespaceWithRenamtedMethods::Typedef resolving to ::SomeOtherNamespace::Class.  I guess my current hasDeclaration-based matcher will match both cases :-(  One way to fix this would be to exclude typedefs in |decl_under_blink_namespace| at https://chromium.googlesource.com/chromium/src/+/14d095b4df6754fa4e6959220b2b332db0b4f504/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp#646
> 
> But... this question+answer should have no impact on the CL under review, right?
> 
> > I'd personally probably have expected (2), but I'm never sure in these cases without playing around with more test cases...
> 
> Ok.  This (#2) is what the current patch results in.
You're right that regardless of what the right solution for your tool is, we should close this hole :)

Richard, can you elaborate on why you would have expected (3) to happen? I'm reluctant to put something into the matchers that you think is unexpected...


https://reviews.llvm.org/D24361