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

BUG=677166

Review-Url: https://codereview.chromium.org/2608443002
Cr-Commit-Position: refs/heads/master@{#440878}

tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

@@ -207,16 +207,24 @@ bool IsMethodOverrideOf(const clang::CXXMethodDecl& decl,
   return false;
 }
 
-bool IsBlacklistedFunction(const clang::FunctionDecl& decl) {
+bool IsBlacklistedFunctionName(llvm::StringRef name) {
+  // https://crbug.com/677166: Have to avoid renaming |hash| -> |Hash| to avoid
+  // colliding with a struct already named |Hash|.
+  return name == "hash";
+}
+
+bool IsBlacklistedFreeFunctionName(llvm::StringRef name) {
   // swap() functions should match the signature of std::swap for ADL tricks.
-  return decl.getName() == "swap";
+  return name == "swap";
 }
 
-bool IsBlacklistedMethodName(llvm::StringRef name) {
+bool IsBlacklistedInstanceMethodName(llvm::StringRef name) {
   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 +233,27 @@ bool IsBlacklistedMethodName(llvm::StringRef name) {
   return false;
 }
 
+bool IsBlacklistedMethodName(llvm::StringRef name) {
+  return IsBlacklistedFunctionName(name) ||
+         IsBlacklistedInstanceMethodName(name);
+}
+
+bool IsBlacklistedFunction(const clang::FunctionDecl& decl) {
+  clang::StringRef name = decl.getName();
+  return IsBlacklistedFunctionName(name) || IsBlacklistedFreeFunctionName(name);
+}
+
 bool IsBlacklistedMethod(const clang::CXXMethodDecl& decl) {
+  clang::StringRef name = decl.getName();
+  if (IsBlacklistedFunctionName(name))
+    return true;
+
+  // Remaining cases are only applicable to instance methods.
   if (decl.isStatic())
     return false;
 
-  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

tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc

@@ -235,6 +235,39 @@ class PaintLayerStackingNode {
 
 }  // namespace get_prefix_vs_inheritance
 
+namespace blacklisting_of_method_and_function_names {
+
+class Foo {
+  // Expecting |swap| method to be renamed to |Swap| - we blacklist renaming of
+  // |swap| *function*, because it needs to have the same casing as std::swap,
+  // so that ADL can kick-in and pull it from another namespace depending on the
+  // bargument.  We have a choice to rename or not rename |swap| *methods* - we
+  // chose to rename to be consistent (i.e. we rename |clear| -> |Clear|) and
+  // because Google C++ Styke Guide uses "Swap" in examples.
+  void Swap() {}
+  static void Swap(Foo& x, Foo& y) {}
+
+  // We don't rename |begin|, so that <algorithms> and other templates that
+  // expect |begin|, |end|, etc. continue to work.  This is only necessary
+  // for instance methods - renaming static methods and funcitons is okay.
+  void begin() {}
+  static void Begin(int x) {}
+
+  // https://crbug.com/677166: We blacklist renaming of |hash|, because it
+  // collides with a struct named Hash.  Blacklisting therefore should be broad
+  // and should cover both instance and static methods as well as functions.
+  int hash() const { return 123; }
+  static int hash(const Foo& x) { return x.hash(); }
+};
+
+void Begin(int x) {}
+int hash(int x) {
+  return 123 * x;
+}
+void swap(Foo& x, Foo& y) {}
+
+}  // blacklisting_of_method_and_function_names
+
 }  // namespace blink
 
 namespace WTF {

tools/clang/rewrite_to_chrome_style/tests/methods-original.cc

@@ -239,6 +239,39 @@ class PaintLayerStackingNode {
 
 }  // namespace get_prefix_vs_inheritance
 
+namespace blacklisting_of_method_and_function_names {
+
+class Foo {
+  // Expecting |swap| method to be renamed to |Swap| - we blacklist renaming of
+  // |swap| *function*, because it needs to have the same casing as std::swap,
+  // so that ADL can kick-in and pull it from another namespace depending on the
+  // bargument.  We have a choice to rename or not rename |swap| *methods* - we
+  // chose to rename to be consistent (i.e. we rename |clear| -> |Clear|) and
+  // because Google C++ Styke Guide uses "Swap" in examples.
+  void swap() {}
+  static void swap(Foo& x, Foo& y) {}
+
+  // We don't rename |begin|, so that <algorithms> and other templates that
+  // expect |begin|, |end|, etc. continue to work.  This is only necessary
+  // for instance methods - renaming static methods and funcitons is okay.
+  void begin() {}
+  static void begin(int x) {}
+
+  // https://crbug.com/677166: We blacklist renaming of |hash|, because it
+  // collides with a struct named Hash.  Blacklisting therefore should be broad
+  // and should cover both instance and static methods as well as functions.
+  int hash() const { return 123; }
+  static int hash(const Foo& x) { return x.hash(); }
+};
+
+void begin(int x) {}
+int hash(int x) {
+  return 123 * x;
+}
+void swap(Foo& x, Foo& y) {}
+
+}  // blacklisting_of_method_and_function_names
+
 }  // namespace blink
 
 namespace WTF {