rewrite_to_chrome_style: Blacklisted methods are not blink methods!

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 13b36000a1e2bbe359079ecb4d05560b0db1343b..dff645c825d17ca7fdc87b49a9fa5ec7487fb5af 100644
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
@@ -64,6 +64,10 @@ 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();
 }
@@ -131,6 +135,15 @@ bool IsBlinkOrWTFMethod(const clang::CXXMethodDecl& decl,
   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))
@@ -305,13 +318,6 @@ bool GetNameForDecl(const clang::CXXMethodDecl& decl,
           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();