Don't rename references to functions that originate from template args.

BUG=595577

Review URL: https://codereview.chromium.org/1806903003

Cr-Commit-Position: refs/heads/master@{#382160}

3 files changed, 191 insertions, 44 deletions

diff --git a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
index 1e22e9c..bac531ea 100644
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
@@ -48,18 +48,6 @@ using llvm::StringRef;
 
 namespace {
 
-// Hack: prevent the custom isDefaulted() from conflicting with the one defined
-// in newer revisions of Clang.
-namespace internal_hack {
-
-// This is available in newer clang revisions... but alas, Chrome has not rolled
-// that far yet.
-AST_MATCHER(clang::FunctionDecl, isDefaulted) {
-  return Node.isDefaulted();
-}
-
-}  // namespace internal_hack
-
 const char kBlinkFieldPrefix[] = "m_";
 const char kBlinkStaticMemberPrefix[] = "s_";
 const char kGeneratedFileRegex[] = "^gen/|/gen/";
@@ -203,7 +191,6 @@ std::string CamelCaseToUnderscoreCase(StringRef input) {
   std::string output;
   bool needs_underscore = false;
   bool was_lowercase = false;
-  bool was_number = false;
   bool was_uppercase = false;
   bool first_char = true;
   // Iterate in reverse to minimize the amount of backtracking.
@@ -211,7 +198,6 @@ std::string CamelCaseToUnderscoreCase(StringRef input) {
        --i) {
     char c = *i;
     bool is_lowercase = clang::isLowercase(c);
-    bool is_number = clang::isDigit(c);
     bool is_uppercase = clang::isUppercase(c);
     c = clang::toLowercase(c);
     // Transitioning from upper to lower case requires an underscore. This is
@@ -230,7 +216,6 @@ std::string CamelCaseToUnderscoreCase(StringRef input) {
     if (!first_char && was_lowercase && is_uppercase)
       needs_underscore = true;
     was_lowercase = is_lowercase;
-    was_number = is_number;
     was_uppercase = is_uppercase;
     first_char = false;
   }
@@ -400,78 +385,79 @@ bool GetNameForDecl(const clang::FunctionTemplateDecl& decl,
   return GetNameForDecl(*templated_function, context, name);
 }
 
-bool GetNameForDecl(const clang::UsingDecl& decl,
+bool GetNameForDecl(const clang::NamedDecl& decl,
                     const clang::ASTContext& context,
                     std::string& name) {
-  assert(decl.shadow_size() > 0);
-
-  // If a using declaration's targeted declaration is a set of overloaded
-  // functions, it can introduce multiple shadowed declarations. Just using the
-  // first one is OK, since overloaded functions have the same name, by
-  // definition.
-  clang::NamedDecl* shadowed_name = decl.shadow_begin()->getTargetDecl();
   // Note: CXXMethodDecl must be checked before FunctionDecl, because
   // CXXMethodDecl is derived from FunctionDecl.
-  if (auto* method = clang::dyn_cast<clang::CXXMethodDecl>(shadowed_name))
+  if (auto* method = clang::dyn_cast<clang::CXXMethodDecl>(&decl))
     return GetNameForDecl(*method, context, name);
-  if (auto* function = clang::dyn_cast<clang::FunctionDecl>(shadowed_name))
+  if (auto* function = clang::dyn_cast<clang::FunctionDecl>(&decl))
     return GetNameForDecl(*function, context, name);
-  if (auto* var = clang::dyn_cast<clang::VarDecl>(shadowed_name))
+  if (auto* var = clang::dyn_cast<clang::VarDecl>(&decl))
     return GetNameForDecl(*var, context, name);
-  if (auto* field = clang::dyn_cast<clang::FieldDecl>(shadowed_name))
+  if (auto* field = clang::dyn_cast<clang::FieldDecl>(&decl))
     return GetNameForDecl(*field, context, name);
   if (auto* function_template =
-          clang::dyn_cast<clang::FunctionTemplateDecl>(shadowed_name))
+          clang::dyn_cast<clang::FunctionTemplateDecl>(&decl))
     return GetNameForDecl(*function_template, context, name);
-  if (auto* enumc = clang::dyn_cast<clang::EnumConstantDecl>(shadowed_name))
+  if (auto* enumc = clang::dyn_cast<clang::EnumConstantDecl>(&decl))
     return GetNameForDecl(*enumc, context, name);
 
   return false;
 }
 
+bool GetNameForDecl(const clang::UsingDecl& decl,
+                    const clang::ASTContext& context,
+                    std::string& name) {
+  assert(decl.shadow_size() > 0);
+
+  // If a using declaration's targeted declaration is a set of overloaded
+  // functions, it can introduce multiple shadowed declarations. Just using the
+  // first one is OK, since overloaded functions have the same name, by
+  // definition.
+  return GetNameForDecl(*decl.shadow_begin()->getTargetDecl(), context, name);
+}
+
 template <typename Type>
 struct TargetNodeTraits;
 
 template <>
 struct TargetNodeTraits<clang::NamedDecl> {
-  static const char kName[];
   static clang::SourceLocation GetLoc(const clang::NamedDecl& decl) {
     return decl.getLocation();
   }
+  static const char* GetName() { return "decl"; }
   static const char* GetType() { return "NamedDecl"; }
 };
-const char TargetNodeTraits<clang::NamedDecl>::kName[] = "decl";
 
 template <>
 struct TargetNodeTraits<clang::MemberExpr> {
-  static const char kName[];
   static clang::SourceLocation GetLoc(const clang::MemberExpr& expr) {
     return expr.getMemberLoc();
   }
+  static const char* GetName() { return "expr"; }
   static const char* GetType() { return "MemberExpr"; }
 };
-const char TargetNodeTraits<clang::MemberExpr>::kName[] = "expr";
 
 template <>
 struct TargetNodeTraits<clang::DeclRefExpr> {
-  static const char kName[];
   static clang::SourceLocation GetLoc(const clang::DeclRefExpr& expr) {
     return expr.getLocation();
   }
+  static const char* GetName() { return "expr"; }
   static const char* GetType() { return "DeclRefExpr"; }
 };
-const char TargetNodeTraits<clang::DeclRefExpr>::kName[] = "expr";
 
 template <>
 struct TargetNodeTraits<clang::CXXCtorInitializer> {
-  static const char kName[];
   static clang::SourceLocation GetLoc(const clang::CXXCtorInitializer& init) {
     assert(init.isWritten());
     return init.getSourceLocation();
   }
+  static const char* GetName() { return "initializer"; }
   static const char* GetType() { return "CXXCtorInitializer"; }
 };
-const char TargetNodeTraits<clang::CXXCtorInitializer>::kName[] = "initializer";
 
 template <typename DeclNode, typename TargetNode>
 class RewriterBase : public MatchFinder::MatchCallback {
@@ -506,7 +492,7 @@ class RewriterBase : public MatchFinder::MatchCallback {
       return;
     clang::SourceLocation loc = TargetNodeTraits<TargetNode>::GetLoc(
         *result.Nodes.getNodeAs<TargetNode>(
-            TargetNodeTraits<TargetNode>::kName));
+            TargetNodeTraits<TargetNode>::GetName()));
     clang::CharSourceRange range = clang::CharSourceRange::getTokenRange(loc);
     replacements_->emplace(*result.SourceManager, range, new_name);
     replacement_names_.emplace(old_name.str(), std::move(new_name));
@@ -608,7 +594,7 @@ int main(int argc, const char* argv[]) {
           // compiler, such as a synthesized copy constructor.
           // This skips explicitly defaulted functions as well, but that's OK:
           // there's nothing interesting to rewrite in those either.
-          unless(hasAncestor(functionDecl(internal_hack::isDefaulted())))));
+          unless(hasAncestor(functionDecl(isDefaulted())))));
   auto decl_ref_matcher = id("expr", declRefExpr(to(var_decl_matcher)));
   auto enum_member_ref_matcher =
       id("expr", declRefExpr(to(enum_member_decl_matcher)));
@@ -660,8 +646,10 @@ int main(int argc, const char* argv[]) {
   //   f();
   //   void (*p)() = &f;
   // matches |f()| and |&f|.
-  auto function_ref_matcher =
-      id("expr", declRefExpr(to(function_decl_matcher)));
+  auto function_ref_matcher = id(
+      "expr", declRefExpr(to(function_decl_matcher),
+                          // Ignore template substitutions.
+                          unless(hasAncestor(substNonTypeTemplateParmExpr()))));
   FunctionRefRewriter function_ref_rewriter(&replacements);
   match_finder.addMatcher(function_ref_matcher, &function_ref_rewriter);
 
@@ -685,8 +673,8 @@ int main(int argc, const char* argv[]) {
               // Overloaded operators have special names and should never be
               // renamed.
               isOverloadedOperator(),
-              // Similarly, constructors, destructors, and conversion functions
-              // should not be considered for renaming.
+              // Similarly, constructors, destructors, and conversion
+              // functions should not be considered for renaming.
               cxxConstructorDecl(), cxxDestructorDecl(), cxxConversionDecl())),
           // Check this last after excluding things, to avoid
           // asserts about overriding non-blink and blink for the
@@ -701,7 +689,10 @@ int main(int argc, const char* argv[]) {
   //   s.g();
   //   void (S::*p)() = &S::g;
   // matches |&S::g| but not |s.g()|.
-  auto method_ref_matcher = id("expr", declRefExpr(to(method_decl_matcher)));
+  auto method_ref_matcher = id(
+      "expr", declRefExpr(to(method_decl_matcher),
+                          // Ignore template substitutions.
+                          unless(hasAncestor(substNonTypeTemplateParmExpr()))));
 
   MethodRefRewriter method_ref_rewriter(&replacements);
   match_finder.addMatcher(method_ref_matcher, &method_ref_rewriter);
diff --git a/tools/clang/rewrite_to_chrome_style/tests/template-expected.cc b/tools/clang/rewrite_to_chrome_style/tests/template-expected.cc
index 39ae4e5..f36b382 100644
--- a/tools/clang/rewrite_to_chrome_style/tests/template-expected.cc
+++ b/tools/clang/rewrite_to_chrome_style/tests/template-expected.cc
@@ -2,6 +2,20 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+namespace not_blink {
+
+void notBlinkFunction(int x) {}
+
+class NotBlinkClass {
+ public:
+  void notBlinkMethod() {}
+};
+
+template <typename T>
+void notBlinkFunctionTemplate(T x) {}
+
+}  // not_blink
+
 namespace blink {
 
 template <typename T, int number>
@@ -20,4 +34,68 @@ void F() {
   const int is_a_const = number;
 }
 
+namespace test_template_arg_is_function {
+
+void F(int x) {}
+
+template <typename T, void g(T)>
+void H(T x) {
+  g(x);
+}
+
+void Test() {
+  // f should be rewritten.
+  H<int, F>(0);
+  // notBlinkFunction should stay the same.
+  H<int, not_blink::notBlinkFunction>(1);
+}
+
+}  // namespace test_template_arg_is_function
+
+namespace test_template_arg_is_method {
+
+class BlinkClass {
+ public:
+  void Method() {}
+};
+
+template <typename T, void (T::*g)()>
+void H(T&& x) {
+  (x.*g)();
+}
+
+void Test() {
+  // method should be rewritten.
+  H<BlinkClass, &BlinkClass::Method>(BlinkClass());
+  H<not_blink::NotBlinkClass, &not_blink::NotBlinkClass::notBlinkMethod>(
+      not_blink::NotBlinkClass());
+}
+
+}  // namespace test_template_arg_is_method
+
+// Test template arguments that are function templates.
+template <typename T, char converter(T)>
+unsigned ReallyBadHash(const T* data, unsigned length) {
+  unsigned hash = 1;
+  for (unsigned i = 0; i < length; ++i) {
+    hash *= converter(data[i]);
+  }
+  return hash;
+}
+
+struct StringHasher {
+  static unsigned Hash(const char* data, unsigned length) {
+    // TODO(dcheng): For some reason, brokenFoldCase gets parsed as an
+    // UnresolvedLookupExpr, so this doesn't get rewritten. Meh. See
+    // https://crbug.com/584408.
+    return ReallyBadHash<char, brokenFoldCase<char>>(data, length);
+  }
+
+ private:
+  template <typename T>
+  static char BrokenFoldCase(T input) {
+    return input - ('a' - 'A');
+  }
+};
+
 }  // namespace blink
diff --git a/tools/clang/rewrite_to_chrome_style/tests/template-original.cc b/tools/clang/rewrite_to_chrome_style/tests/template-original.cc
index 39ae4e5..cb22517 100644
--- a/tools/clang/rewrite_to_chrome_style/tests/template-original.cc
+++ b/tools/clang/rewrite_to_chrome_style/tests/template-original.cc
@@ -2,6 +2,20 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+namespace not_blink {
+
+void notBlinkFunction(int x) {}
+
+class NotBlinkClass {
+ public:
+  void notBlinkMethod() {}
+};
+
+template <typename T>
+void notBlinkFunctionTemplate(T x) {}
+
+}  // not_blink
+
 namespace blink {
 
 template <typename T, int number>
@@ -20,4 +34,68 @@ void F() {
   const int is_a_const = number;
 }
 
+namespace test_template_arg_is_function {
+
+void f(int x) {}
+
+template <typename T, void g(T)>
+void h(T x) {
+  g(x);
+}
+
+void test() {
+  // f should be rewritten.
+  h<int, f>(0);
+  // notBlinkFunction should stay the same.
+  h<int, not_blink::notBlinkFunction>(1);
+}
+
+}  // namespace test_template_arg_is_function
+
+namespace test_template_arg_is_method {
+
+class BlinkClass {
+ public:
+  void method() {}
+};
+
+template <typename T, void (T::*g)()>
+void h(T&& x) {
+  (x.*g)();
+}
+
+void test() {
+  // method should be rewritten.
+  h<BlinkClass, &BlinkClass::method>(BlinkClass());
+  h<not_blink::NotBlinkClass, &not_blink::NotBlinkClass::notBlinkMethod>(
+      not_blink::NotBlinkClass());
+}
+
+}  // namespace test_template_arg_is_method
+
+// Test template arguments that are function templates.
+template <typename T, char converter(T)>
+unsigned reallyBadHash(const T* data, unsigned length) {
+  unsigned hash = 1;
+  for (unsigned i = 0; i < length; ++i) {
+    hash *= converter(data[i]);
+  }
+  return hash;
+}
+
+struct StringHasher {
+  static unsigned Hash(const char* data, unsigned length) {
+    // TODO(dcheng): For some reason, brokenFoldCase gets parsed as an
+    // UnresolvedLookupExpr, so this doesn't get rewritten. Meh. See
+    // https://crbug.com/584408.
+    return reallyBadHash<char, brokenFoldCase<char>>(data, length);
+  }
+
+ private:
+  template <typename T>
+  static char brokenFoldCase(T input) {
+    return input - ('a' - 'A');
+  }
+};
+
 }  // namespace blink