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

tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // Changes Blink-style names to Chrome-style names. Currently transforms:
 //   fields:
 //     int m_operationCount => int operation_count_
 //   variables (including parameters):
 //     int mySuperVariable => int my_super_variable
 //   constants:
@@ skipping 169 matching lines @@
 // Matches |T::m| DependentScopeDeclRefExpr if InnerMatcher matches |T|.
 AST_MATCHER_P(
     clang::DependentScopeDeclRefExpr,
     dependentScopeIfQualifier,
     clang::ast_matchers::internal::Matcher<clang::NestedNameSpecifier>,
     InnerMatcher) {
   clang::NestedNameSpecifier* qual = Node.getQualifier();
   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,
               clang::ast_matchers::internal::Matcher<clang::Type>,
               InnerMatcher) {
   const clang::Type* type = Node.getTypePtrOrNull();
   return type && InnerMatcher.matches(*type, Finder, Builder);
 }
 
 bool IsMethodOverrideOf(const clang::CXXMethodDecl& decl,
@@ skipping 605 matching lines @@
   llvm::InitializeNativeTargetAsmParser();
   llvm::cl::OptionCategory category(
       "rewrite_to_chrome_style: convert Blink style to Chrome style.");
   CommonOptionsParser options(argc, argv, category);
   clang::tooling::ClangTool tool(options.getCompilations(),
                                  options.getSourcePathList());
 
   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
   //   int x;
   //   struct S {
   //     int y;
   //     enum { VALUE };
   //   };
   // matches |x|, |y|, and |VALUE|.
   auto field_decl_matcher = id("decl", fieldDecl(in_blink_namespace));
@@ skipping 352 matching lines @@
   for (const auto& r : replacements) {
     std::string replacement_text = r.getReplacementText().str();
     std::replace(replacement_text.begin(), replacement_text.end(), '\n', '\0');
     llvm::outs() << "r:::" << r.getFilePath() << ":::" << r.getOffset()
                  << ":::" << r.getLength() << ":::" << replacement_text << "\n";
   }
   llvm::outs() << "==== END EDITS ====\n";
 
   return 0;
 }