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