Tweak |in_blink_namespace|, to look at ancestors *and also* at qualifiers.

tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

Index: tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
diff --git a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
index 267f617258d5e23e1edd491c175af87160939f15..3a19515c190ac61352db73b703e20e33987e67ac 100644
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
@@ -187,6 +187,16 @@ AST_MATCHER_P(
   return qual && InnerMatcher.matches(*qual, Finder, Builder);
 }
 
+// Matches |T::f| DependentScopeDeclRefExpr if InnerMatcher matches |T|.
+AST_MATCHER_P(
+    clang::DeclaratorDecl,
+    declaratorDeclHasQualifier,
+    clang::ast_matchers::internal::Matcher<clang::NestedNameSpecifier>,
+    InnerMatcher) {

[dcheng, 2016/08/25 00:33:12]

Is there any chance we can use the nestedNameSpecifier() matcher? There's some
examples in the clang tests that suggest this might work:
https://cs.chromium.org/chromium/src/third_party/llvm/tools/clang/unittests/A...

[Łukasz Anforowicz, 2016/08/25 21:08:09]

Interesting.

FWIW, the AST for ...

    template <typename Allocator = PartitionAllocator>
    void test() {
      // https://crbug.com/582315: |Allocator::method| is a
      // DependentScopeDeclRefExpr.
      if (std::is_class<Allocator>::value)  // Shouldn't rename |value|.
        Allocator::method();                // Should rename |method|.
    }

... looks like this:

    |-FunctionTemplateDecl 0x5867738 <line:215:1, line:221:1> line:216:6 test
    | |-TemplateTypeParmDecl 0x58675f0 <line:215:11, col:32> col:20 referenced
typename Allocator
    | | `-TemplateArgument type 'class
blink::test_new_array_bug::PartitionAllocator'
    | `-FunctionDecl 0x5867690 <line:216:1, line:221:1> line:216:6 test 'void
(void)'
    |   `-CompoundStmt 0x5867a20 <col:13, line:221:1>
    |     `-IfStmt 0x58679e8 <line:219:3, line:220:23>
    |       |-<<<NULL>>>
    |       |-<<<NULL>>>
    |       |-DependentScopeDeclRefExpr 0x58678d8 <line:219:7, col:33>
'<dependent type>' lvalue
    |       |-CallExpr 0x58679c0 <line:220:5, col:23> '<dependent type>'
    |       | `-DependentScopeDeclRefExpr 0x5867980 <col:5, col:16> '<dependent
type>' lvalue
    |       `-<<<NULL>>>

Note that, there is no NestedNameSpecifier node above.  But... the following
*does* work:

-     declaratorDecl(declaratorDeclHasQualifier(...
+     declaratorDecl(has(nestedNameSpecifier(...

Done.  Thanks for the suggestion.

PS. I can probably also apply this suggestion to the other places where I
introduce hasQualifier-like matchers - in
https://codereview.chromium.org/2256913002.  I'll give it a try in a minute.

+  clang::NestedNameSpecifier* qual = Node.getQualifier();
+  return qual && InnerMatcher.matches(*qual, Finder, Builder);
+}
+
 // Matches |const Class<T>&| QualType if InnerMatcher matches |Class<T>|.
 AST_MATCHER_P(clang::QualType,
               hasUnderlyingType,
@@ -812,10 +822,29 @@ int main(int argc, const char* argv[]) {
   MatchFinder match_finder;
   std::set<Replacement> replacements;
 
-  auto in_blink_namespace =
-      decl(hasAncestor(namespaceDecl(anyOf(hasName("blink"), hasName("WTF")),
-                                     hasParent(translationUnitDecl()))),
-           unless(isExpansionInFileMatching(kGeneratedFileRegex)));
+  // Blink namespace matchers ========
+  auto blink_namespace_decl =
+      namespaceDecl(anyOf(hasName("blink"), hasName("WTF")),
+                    hasParent(translationUnitDecl()));
+
+  // Given top-level compilation unit:
+  //   namespace WTF {
+  //     void foo() {}
+  //   }
+  // matches |foo|.
+  auto decl_under_blink_namespace = decl(hasAncestor(blink_namespace_decl));
+
+  // Given top-level compilation unit:
+  //   void WTF::foo() {}
+  // matches |WTF::foo|.
+  auto decl_has_qualifier_to_blink_namespace =
+      declaratorDecl(declaratorDeclHasQualifier(
+          anyOf(specifiesNamespace(blink_namespace_decl),
+                hasPrefix(specifiesNamespace(blink_namespace_decl)))));

[dcheng, 2016/08/25 00:33:12]

How come we need both? It would be good to expand the comment to make the reason
clear.

[Łukasz Anforowicz, 2016/08/25 21:08:09]

I've added a test in methods-original.cc + verified that
1. with only specifiesNamespace(...), WTF::Class::method doesn't get rewritten
(in the newly added test)
2. with only hasPrefix(specifiesNamespace(...)), WTF::function doesn't get
rewritten (in the test present in the previous patchset)

I've expanded the comment above to clarify which parts of the matcher are
responsible for which parts of the matched code.

WDYT?

PS. I wondered whether to split the matcher, but I am not sure if more verbose
code would be cleaner.  Please shout if you think that matching (1) and (2) in
two pieces of the comments is not readable (the alternative being splitting the
matcher into 2 halves, right? :-/ ).

+
+  auto in_blink_namespace = decl(
+      anyOf(decl_under_blink_namespace, decl_has_qualifier_to_blink_namespace),
+      unless(isExpansionInFileMatching(kGeneratedFileRegex)));
 
   // Field, variable, and enum declarations ========
   // Given