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

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 189 matching lines @@
   if (decl.getParent()->getQualifiedNameAsString() == class_name)
     return true;
   for (auto it = decl.begin_overridden_methods();
        it != decl.end_overridden_methods(); ++it) {
     if (IsMethodOverrideOf(**it, class_name))
       return true;
   }
   return false;
 }
 
-bool IsBlacklistedFunction(const clang::FunctionDecl& decl) {
-  // swap() functions should match the signature of std::swap for ADL tricks.
-  return decl.getName() == "swap";
+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 IsBlacklistedMethodName(llvm::StringRef name) {
+bool IsBlacklistedFreeFunctionName(llvm::StringRef name) {
+  // swap() functions should match the signature of std::swap for ADL tricks.
+  return name == "swap";
+}
+
+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)
       return true;
   }
   return false;
 }
 
+bool IsBlacklistedFunction(const clang::FunctionDecl& decl) {
+  clang::StringRef name = decl.getName();
+  return IsBlacklistedFunctionName(name) || IsBlacklistedFreeFunctionName(name);
+}
+
 bool IsBlacklistedMethod(const clang::CXXMethodDecl& decl) {
-  if (decl.isStatic())
-    return false;
+  clang::StringRef name = decl.getName();
+  if (IsBlacklistedFunctionName(name))
+    return true;
 
-  clang::StringRef name = decl.getName();
-  if (IsBlacklistedMethodName(name))
-      return true;
+  if (decl.isInstance() && IsBlacklistedInstanceMethodName(name))
+    return true;

[dcheng, 2016/12/28 18:48:49]

I think we can just early return here if it's static, right?

[Łukasz Anforowicz, 2016/12/28 18:56:50]

Done.

 
   // Subclasses of InspectorAgent will subclass "disable()" from both blink and
   // from gen/, which is problematic, but DevTools folks don't want to rename
   // it or split this up. So don't rename it at all.
-  if (name.equals("disable") &&
+  if (decl.isInstance() && name.equals("disable") &&
       IsMethodOverrideOf(decl, "blink::InspectorAgent"))
     return true;
 
   return false;
 }
 
 AST_MATCHER(clang::FunctionDecl, isBlacklistedFunction) {
   return IsBlacklistedFunction(Node);
 }
 
@@ skipping 595 matching lines @@
     llvm::StringRef old_name = info->getName();
 
     // Try to guess a new name.
     std::string new_name;
     if (GuessNameForUnresolvedDependentNode(node, *result.Context, old_name,
                                             new_name))
       Base::AddReplacement(result, old_name, std::move(new_name));
   }
 
  private:
+  static bool IsBlacklistedMethodName(llvm::StringRef name) {

[dcheng, 2016/12/28 18:48:49]

How come this is a static method now?

[Łukasz Anforowicz, 2016/12/28 18:56:50]

This was a free function, but the only caller was/is
GuessNameForUnresolvedDependentNode below, so I think it is cleaner if
IsBlacklistedMethodName is a static, private method.

Maybe it would help if I explained that IsBlacklistedMethodName covers names of
both instance and static methods?  Such combination is not covered by any of the
other blacklisting helpers above.

[dcheng, 2016/12/28 19:03:38]

I think it's just confusing to not have all the similar helpers grouped
together. I think it's OK that it's only really used by one thing.

[Łukasz Anforowicz, 2016/12/28 19:08:07]

Done.

+    return IsBlacklistedFunctionName(name) ||
+           IsBlacklistedInstanceMethodName(name);
+  }
+
   // This method calculates a new name for nodes that depend on template
   // parameters (http://en.cppreference.com/w/cpp/language/dependent_name).  The
   // renaming is based on crude heuristics, because such nodes are not bound to
   // a specific decl until template instantiation - at the point of rename, one
   // cannot tell whether the node will eventually resolve to a field / method /
   // constant / etc.
   //
   // The method returns false if no renaming should be done.
   // Otherwise the method returns true and sets |new_name|.
   bool GuessNameForUnresolvedDependentNode(const TargetNode& node,
@@ skipping 429 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;
 }