rewrite_to_chrome_style: Blacklisted methods are not blink methods!

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 46 matching lines @@
 AST_MATCHER(clang::FunctionDecl, isDefaulted) {
   return Node.isDefaulted();
 }
 
 }  // namespace internal_hack
 
 const char kBlinkFieldPrefix[] = "m_";
 const char kBlinkStaticMemberPrefix[] = "s_";
 const char kGeneratedFileRegex[] = "^gen/|/gen/";
 
+// These methods should never be renamed, we do not consider them
+// to be blink (or wtf) methods at all.
+const char* const kBlacklistMethods[] = {"trace", "lock", "unlock", "try_lock"};
+
 AST_MATCHER(clang::FunctionDecl, isOverloadedOperator) {
   return Node.isOverloadedOperator();
 }
 
 AST_MATCHER_P(clang::FunctionTemplateDecl,
               templatedDecl,
               clang::ast_matchers::internal::Matcher<clang::FunctionDecl>,
               InnerMatcher) {
   return InnerMatcher.matches(*Node.getTemplatedDecl(), Finder, Builder);
 }
@@ skipping 47 matching lines @@
   return false;
 }
 
 // A method is from Blink if it is from the Blink namespace or overrides a
 // method from the Blink namespace.
 bool IsBlinkOrWTFMethod(const clang::CXXMethodDecl& decl,
                         clang::SourceManager& source_manager) {
   bool overrides_blink = false;
   bool overrides_non_blink = false;
 
+  // Blacklisted methods are not considered to be in blink so that they will

[dcheng, 2016/03/10 22:52:46]

1) Would it make sense to just combine the iterator blacklist and this and have
it in the same place?

2) Would it make sense to express this in the matcher instead?

unless(anyOf(<really long list of current exceptions>,
    hasName("begin"),
    hasName("end"),
    hasName("rbegin"),
    hasName("rend"),
    ...))
so on and so forth.

[danakj, 2016/03/10 22:55:14]

We made the iterator blacklist only kick in based on the return type being an
iterator name though. That doesn't get expressed as easily with matchers (or IDK
how atm).

We could do that up here instead, but it'll be a separate list of methods still.
Prefer that?

[dcheng, 2016/03/10 22:57:53]

Hmm... that makes it more complicated.

I'd suggest putting it (including the iterator stuff) in a helper function like
IsBlacklistedMethod() and calling that from here then. Or if you want to be
really fancy, you can make it a custom matcher like isBlacklistedMethod().

+  // never be rewritten.
+  if (!decl.isStatic()) {
+    for (const auto& b : kBlacklistMethods) {
+      if (decl.getNameAsString() == b)
+        return false;
+    }
+  }
+
   for (auto it = decl.begin_overridden_methods();
        it != decl.end_overridden_methods(); ++it) {
     if (IsBlinkOrWTFMethod(**it, source_manager))
       overrides_blink = true;
     else
       overrides_non_blink = true;
   }
 
   // If this fires we have a class overriding a method from a class in blink,
   // and also overriding a method of the same name from a class not in blink,
@@ skipping 154 matching lines @@
         ret_type.find("Iterator") != std::string::npos) {
       // Iterator methods shouldn't be renamed to work with stl and range-for
       // loops.
       static const char* kIteratorBlacklist[] = {"begin", "end", "rbegin",
                                                  "rend"};
       for (const auto& b : kIteratorBlacklist) {
         if (original_name == b)
           return false;
       }
     }
-
-    // Some methods shouldn't be renamed because reasons.
-    static const char* kBlacklist[] = {"trace", "lock", "unlock", "try_lock"};
-    for (const auto& b : kBlacklist) {
-      if (original_name == b)
-        return false;
-    }
   }
 
   name = decl.getName().str();
   name[0] = clang::toUppercase(name[0]);
   return true;
 }
 
 bool GetNameForDecl(const clang::FieldDecl& decl,
                     const clang::ASTContext& context,
                     clang::SourceManager& source_manager,
@@ skipping 466 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;
 }