Blacklisting renaming of "hash" in case of static methods and functions.

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 3374ea29ea6bd6ab8c3f0b796c90c6e5683384c2..626872b4948387a2aa5c4319e80e6f9002095493 100644
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
@@ -207,16 +207,27 @@ bool IsMethodOverrideOf(const clang::CXXMethodDecl& decl,
   return false;
 }
 
-bool IsBlacklistedFunction(const clang::FunctionDecl& decl) {
+bool IsBlacklistedFunctionOrInstanceOrStaticMethodName(llvm::StringRef name) {

[dcheng, 2016/12/27 20:30:40]

I find the naming of this helper to be a bit confusing. In clang parlance, I
would expect this to just be called IsBlacklistedFunctionName().

[Łukasz Anforowicz, 2016/12/27 22:42:43]

Done.

+  // https://crbug.com/677166: Have to avoid renaming |hash| -> |Hash| to avoid
+  // colliding with a struct already named |Hash|.
+  return name == "hash";
+}
+
+bool IsBlacklistedFunctionName(llvm::StringRef name) {
   // swap() functions should match the signature of std::swap for ADL tricks.
-  return decl.getName() == "swap";
+  return name == "swap";

[dcheng, 2016/12/27 20:30:40]

And call this "IsBlacklistedFreeFunctionName" (since in clang parlance, methods
are a subclass of functions, while this explicitly excludes checking against
method names)

[Łukasz Anforowicz, 2016/12/27 22:42:43]

Done.

 }
 
-bool IsBlacklistedMethodName(llvm::StringRef name) {
+bool IsBlacklistedInstanceMethodName(llvm::StringRef name) {
+  if (IsBlacklistedFunctionOrInstanceOrStaticMethodName(name))
+    return true;
+
   static const char* kBlacklistedNames[] = {
-      "hash",
-      "lock", "unlock", "try_lock",
-      "begin", "end", "rbegin", "rend",
+      // We should avoid renaming the method names listed below, because
+      // 1. They are used in templated code (e.g. in <algorithms>)
+      // 2. They (begin+end) are used in range-based for syntax sugar
+      //    - for (auto x : foo) { ... }  // <- foo.begin() will be called.
+      "begin", "end", "rbegin", "rend", "lock", "unlock", "try_lock",
   };
   for (const auto& b : kBlacklistedNames) {
     if (name == b)
@@ -225,13 +236,28 @@ bool IsBlacklistedMethodName(llvm::StringRef name) {
   return false;
 }
 
+bool IsBlacklistedStaticMethodName(llvm::StringRef name) {
+  return IsBlacklistedFunctionOrInstanceOrStaticMethodName(name);
+}
+
+bool IsBlacklistedMethodName(llvm::StringRef name) {
+  return IsBlacklistedStaticMethodName(name) ||
+         IsBlacklistedInstanceMethodName(name);
+}
+
+bool IsBlacklistedFunction(const clang::FunctionDecl& decl) {
+  clang::StringRef name = decl.getName();
+  return IsBlacklistedFunctionOrInstanceOrStaticMethodName(name) ||
+         IsBlacklistedFunctionName(name);
+}
+
 bool IsBlacklistedMethod(const clang::CXXMethodDecl& decl) {
+  clang::StringRef name = decl.getName();
   if (decl.isStatic())
-    return false;
+    return IsBlacklistedStaticMethodName(name);
 
-  clang::StringRef name = decl.getName();
-  if (IsBlacklistedMethodName(name))
-      return true;
+  if (IsBlacklistedInstanceMethodName(name))
+    return true;
 
   // Subclasses of InspectorAgent will subclass "disable()" from both blink and
   // from gen/, which is problematic, but DevTools folks don't want to rename