Update rewrite_to_chrome_style tool to also rename 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 42792382264884cb3775ab8ae16f3da27f46769b..fc710dd73fa841102110487159451e65cc6ebfc6 100644
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
@@ -5,16 +5,18 @@
 // Changes Blink-style names to Chrome-style names. Currently transforms:
 //   fields:
 //     int m_operationCount => int operation_count_
-//   variables:
+//   variables (including parameters):
 //     int mySuperVariable => int my_super_variable
 //   constants:
 //     const int maxThings => const int kMaxThings

[danakj, 2016/01/06 22:09:40]

there are at least some constants named MaxThings. will it catch them?

[dcheng, 2016/01/08 01:41:11]

It should (the heuristics for using kConstantStyleNaming aren't based on the
existing name of the variable declaration)

-//   free functions:
+//   free functions and methods:
 //     void doThisThenThat() => void DoThisAndThat()
 
+#include <assert.h>
 #include <algorithm>
 #include <memory>
 #include <string>
+#include <unordered_set>
 
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
@@ -38,12 +40,14 @@ using llvm::StringRef;
 
 namespace {
 
+AST_MATCHER(clang::FunctionDecl, isOverloadedOperator) {
+  return Node.isOverloadedOperator();
+}
+
 constexpr char kBlinkFieldPrefix[] = "m_";
 constexpr char kBlinkStaticMemberPrefix[] = "s_";
 
-bool GetNameForDecl(const clang::FunctionDecl& decl,
-                    clang::ASTContext* context,
-                    std::string& name) {
+bool GetNameForDecl(const clang::FunctionDecl& decl, std::string& name) {
   name = decl.getNameAsString();
   name[0] = clang::toUppercase(name[0]);
   return true;
@@ -83,9 +87,7 @@ std::string CamelCaseToUnderscoreCase(StringRef input) {
   return output;
 }
 
-bool GetNameForDecl(const clang::FieldDecl& decl,
-                    clang::ASTContext* context,
-                    std::string& name) {
+bool GetNameForDecl(const clang::FieldDecl& decl, std::string& name) {
   StringRef original_name = decl.getName();
   // Blink style field names are prefixed with `m_`. If this prefix isn't
   // present, assume it's already been converted to Google style.
@@ -137,9 +139,7 @@ bool IsProbablyConst(const clang::VarDecl& decl) {
          clang::isa<clang::UserDefinedLiteral>(initializer);
 }
 
-bool GetNameForDecl(const clang::VarDecl& decl,
-                    clang::ASTContext* context,
-                    std::string& name) {
+bool GetNameForDecl(const clang::VarDecl& decl, std::string& name) {
   StringRef original_name = decl.getName();
 
   // Nothing to do for unnamed parameters.
@@ -173,71 +173,165 @@ bool GetNameForDecl(const clang::VarDecl& decl,
   return true;
 }
 
-template <typename DeclNodeType, typename TargetTraits>
-class RewriterCallbackBase : public MatchFinder::MatchCallback {
- public:
-  explicit RewriterCallbackBase(Replacements* replacements)
-      : replacements_(replacements) {}
-  virtual void run(const MatchFinder::MatchResult& result) override {
-    std::string name;
-    if (!GetNameForDecl(*result.Nodes.getNodeAs<DeclNodeType>("decl"),
-                        result.Context, name))
-      return;
-    replacements_->emplace(
-        *result.SourceManager,
-        TargetTraits::GetRange(
-            *result.Nodes.getNodeAs<typename TargetTraits::NodeType>(
-                TargetTraits::kName)),
-        name);
-  }
+template <typename Type>
+struct TargetNodeTraits;
 
- private:
-  Replacements* const replacements_;
-};
-
-struct DeclTargetTraits {
-  using NodeType = clang::NamedDecl;
+template <>
+struct TargetNodeTraits<clang::NamedDecl> {
   static constexpr char kName[] = "decl";
-  static clang::CharSourceRange GetRange(const NodeType& decl) {
+  static clang::CharSourceRange GetRange(const clang::NamedDecl& decl) {
     return clang::CharSourceRange::getTokenRange(decl.getLocation());
   }
 };
-constexpr char DeclTargetTraits::kName[];
+constexpr char TargetNodeTraits<clang::NamedDecl>::kName[];
 
-struct ExprTargetTraits {
-  using NodeType = clang::Expr;
+template <>
+struct TargetNodeTraits<clang::CallExpr> {
   static constexpr char kName[] = "expr";
-  static clang::CharSourceRange GetRange(const NodeType& expr) {
+  static clang::CharSourceRange GetRange(const clang::CallExpr& expr) {
     return clang::CharSourceRange::getTokenRange(expr.getExprLoc());
   }
 };
-constexpr char ExprTargetTraits::kName[];
+constexpr char TargetNodeTraits<clang::CallExpr>::kName[];
+
+template <>
+struct TargetNodeTraits<clang::MemberExpr> {
+  static constexpr char kName[] = "expr";
+  static clang::CharSourceRange GetRange(const clang::MemberExpr& expr) {
+    return clang::CharSourceRange::getTokenRange(expr.getMemberLoc());
+  }
+};
+constexpr char TargetNodeTraits<clang::MemberExpr>::kName[];
 
-struct InitializerTargetTraits {
-  using NodeType = clang::CXXCtorInitializer;
+template <>
+struct TargetNodeTraits<clang::DeclRefExpr> {
+  static constexpr char kName[] = "expr";
+  static clang::CharSourceRange GetRange(const clang::DeclRefExpr& expr) {
+    return clang::CharSourceRange::getTokenRange(expr.getLocation());

[danakj, 2016/01/06 22:09:40]

consider a comment on each of these why you chose that .get___() method? Like
why not getExptrLoc() here, but you do above? DeclRefExpr sounds like it could
go either way

[dcheng, 2016/01/08 01:41:11]

I'm not sure how helpful a comment would be. It's basically "this is the
function that returned the right SourceLocation to modify". The documentation
for clang often doesn't really elaborate. For example,
DeclRefExpr::getLocation() has no comment:
http://clang.llvm.org/doxygen/classclang_1_1DeclRefExpr.html#ab4616545c45fc95...
So it's really determined through trial and error.

I do plan on updating the clang tool refactoring docs, and I'll try to go into a
bit of detail about the different kind of source locations (as they've been
quite confusing to me in the past as well: in addition to the myriad of
SourceLocation getters, there's also a 'spelling location' and 'expansion
location' concept, which we invoke rather haphazardly when it seems
'necessary').

+  }
+};
+constexpr char TargetNodeTraits<clang::DeclRefExpr>::kName[];
+
+template <>
+struct TargetNodeTraits<clang::CXXCtorInitializer> {
   static constexpr char kName[] = "initializer";
-  static clang::CharSourceRange GetRange(const NodeType& init) {
+  static clang::CharSourceRange GetRange(
+      const clang::CXXCtorInitializer& init) {
     return clang::CharSourceRange::getTokenRange(init.getSourceLocation());
   }
 };
-constexpr char InitializerTargetTraits::kName[];
+constexpr char TargetNodeTraits<clang::CXXCtorInitializer>::kName[];
+
+template <typename DeclNode, typename TargetNode>
+class RewriterBase : public MatchFinder::MatchCallback {
+ public:
+  explicit RewriterBase(Replacements* replacements)
+      : replacements_(replacements) {}
 
-using FunctionDeclRewriterCallback =
-    RewriterCallbackBase<clang::FunctionDecl, DeclTargetTraits>;
-using FieldDeclRewriterCallback =
-    RewriterCallbackBase<clang::FieldDecl, DeclTargetTraits>;
-using VarDeclRewriterCallback =
-    RewriterCallbackBase<clang::VarDecl, DeclTargetTraits>;
+  void run(const MatchFinder::MatchResult& result) override {
+    std::string name;
+    if (!GetNameForDecl(*result.Nodes.getNodeAs<DeclNode>("decl"), name))
+      return;
+    replacements_->emplace(*result.SourceManager,
+                           TargetNodeTraits<TargetNode>::GetRange(
+                               *result.Nodes.getNodeAs<TargetNode>(
+                                   TargetNodeTraits<TargetNode>::kName)),
+                           name);
+  }
 
-using CallRewriterCallback =
-    RewriterCallbackBase<clang::FunctionDecl, ExprTargetTraits>;
-using MemberRewriterCallback =
-    RewriterCallbackBase<clang::FieldDecl, ExprTargetTraits>;
-using DeclRefRewriterCallback =
-    RewriterCallbackBase<clang::VarDecl, ExprTargetTraits>;
+ private:
+  Replacements* const replacements_;
+};
 
-using ConstructorInitializerRewriterCallback =
-    RewriterCallbackBase<clang::FieldDecl, InitializerTargetTraits>;
+using FieldDeclRewriter = RewriterBase<clang::FieldDecl, clang::NamedDecl>;
+using VarDeclRewriter = RewriterBase<clang::VarDecl, clang::NamedDecl>;
+using MemberRewriter = RewriterBase<clang::FieldDecl, clang::MemberExpr>;
+using DeclRefRewriter = RewriterBase<clang::VarDecl, clang::DeclRefExpr>;
+using ConstructorInitializerRewriter =
+    RewriterBase<clang::FieldDecl, clang::CXXCtorInitializer>;
+
+// Helpers for rewriting functions. The tool needs to detect overrides of Blink
+// methods, and uses two matchers to help accomplish this goal:
+// - The first matcher matches all method declarations in Blink. When the
+//   callback rewrites the declaration, it also stores a pointer to the
+//   canonical declaration, to record it as a Blink method.
+// - The second matcher matches all method declarations that are overrides. When
+//   the callback processes the match, it checks if its overriding a method that
+//   was marked as a Blink method. If so, it rewrites the declaration.
+// - Because an override is determined based on inclusion in the set of Blink
+//   methods, the overridden methods matcher does not need to filter out special
+//   member functions: they get filtered out by virtue of the first matcher.
+//
+// This works because per the documentation on MatchFinder:
+//   The order of matches is guaranteed to be equivalent to doing a pre-order
+//   traversal on the AST, and applying the matchers in the order in which they
+//   were added to the MatchFinder.
+//
+// Since classes cannot forward declare their base classes, it is guaranteed
+// that the base class methods will be seen before processing the overridden
+// methods.
+class FunctionDeclRewriter
+    : public RewriterBase<clang::FunctionDecl, clang::NamedDecl> {
+ public:
+  explicit FunctionDeclRewriter(Replacements* replacements)
+      : RewriterBase(replacements) {}
+
+  void run(const MatchFinder::MatchResult& result) override {
+    if (const clang::CXXMethodDecl* method_decl =
+            result.Nodes.getNodeAs<clang::CXXMethodDecl>("decl")) {
+      // TODO(dcheng): Does this need to check for the override attribute, or is
+      // this good enough?
+      if (method_decl->size_overridden_methods() > 0) {
+        if (!IsBlinkOverride(method_decl))
+          return;
+      } else {
+        blink_methods_.emplace(method_decl->getCanonicalDecl());
+      }
+    }
+
+    RewriterBase::run(result);
+  }
+
+  bool IsBlinkOverride(const clang::CXXMethodDecl* decl) const {
+    assert(decl->size_overridden_methods() > 0);
+    for (auto it = decl->begin_overridden_methods();
+         it != decl->end_overridden_methods(); ++it) {
+      if (blink_methods_.find((*it)->getCanonicalDecl()) !=
+          blink_methods_.end())
+        return true;
+    }
+    return false;
+  }
+
+ private:
+  std::unordered_set<const clang::CXXMethodDecl*> blink_methods_;
+};
+
+template <typename BaseRewriter>
+class FilteringFunctionRewriter : public BaseRewriter {
+ public:
+  FilteringFunctionRewriter(const FunctionDeclRewriter& decl_rewriter,
+                            Replacements* replacements)
+      : BaseRewriter(replacements), decl_rewriter_(decl_rewriter) {}
+
+  void run(const MatchFinder::MatchResult& result) override {
+    if (const clang::CXXMethodDecl* method_decl =
+            result.Nodes.getNodeAs<clang::CXXMethodDecl>("decl")) {
+      if (method_decl->size_overridden_methods() > 0 &&
+          !decl_rewriter_.IsBlinkOverride(method_decl))
+        return;
+    }
+    BaseRewriter::run(result);
+  }
+
+ private:
+  const FunctionDeclRewriter& decl_rewriter_;
+};
+
+using CallRewriter = FilteringFunctionRewriter<
+    RewriterBase<clang::FunctionDecl, clang::CallExpr>>;
+using FunctionDeclRefRewriter = FilteringFunctionRewriter<
+    RewriterBase<clang::FunctionDecl, clang::DeclRefExpr>>;
 
 }  // namespace
 
@@ -260,50 +354,77 @@ int main(int argc, const char* argv[]) {
   auto in_blink_namespace =
       decl(hasAncestor(namespaceDecl(anyOf(hasName("blink"), hasName("WTF")))));
 
-  // Declaration handling (e.g. function definitions and variable definitions):
-
-  // Note: for now, only rewrite standalone functions until the question of JS
-  // binding integration for class methods is resolved.
-  // TODO(dcheng): Since classes in public/ aren't directly web-exposed, just go
-  // ahead and rewrite those.
-  auto function_decl_matcher =
-      id("decl", functionDecl(unless(cxxMethodDecl()), in_blink_namespace));
-
+  // Declarations of fields and variables:
   auto field_decl_matcher = id("decl", fieldDecl(in_blink_namespace));
   auto var_decl_matcher = id("decl", varDecl(in_blink_namespace));
 
-  FunctionDeclRewriterCallback function_decl_rewriter(&replacements);
-  match_finder.addMatcher(function_decl_matcher, &function_decl_rewriter);
-  FieldDeclRewriterCallback field_decl_rewriter(&replacements);
+  FieldDeclRewriter field_decl_rewriter(&replacements);
   match_finder.addMatcher(field_decl_matcher, &field_decl_rewriter);
-  VarDeclRewriterCallback var_decl_rewriter(&replacements);
+
+  VarDeclRewriter var_decl_rewriter(&replacements);
   match_finder.addMatcher(var_decl_matcher, &var_decl_rewriter);
 
-  // Expression handling (e.g. calling a Blink function or referencing a
-  // variable defined in Blink):
-  auto call_matcher = id("expr", callExpr(callee(function_decl_matcher)));
+  // References to declared fields and variables:
   auto member_matcher = id("expr", memberExpr(member(field_decl_matcher)));
   auto decl_ref_matcher = id("expr", declRefExpr(to(var_decl_matcher)));
 
-  CallRewriterCallback call_rewriter(&replacements);
-  match_finder.addMatcher(call_matcher, &call_rewriter);
-  MemberRewriterCallback member_rewriter(&replacements);
+  MemberRewriter member_rewriter(&replacements);
   match_finder.addMatcher(member_matcher, &member_rewriter);
-  DeclRefRewriterCallback decl_ref_rewriter(&replacements);
+
+  DeclRefRewriter decl_ref_rewriter(&replacements);
   match_finder.addMatcher(decl_ref_matcher, &decl_ref_rewriter);
 
-  // Function reference handling (e.g. getting a pointer to a function without
-  // calling it):
+  // The AST matchers don't provide a good way to match against an override from
+  // a given class. Instead, use two matchers: one that matches the declarations
+  // (in Blink) and another which matches all overrides and performs some
+  // filtering.
+  auto function_decl_matcher =
+      id("decl",
+         functionDecl(unless(anyOf(cxxMethodDecl(isOverride()),
+                                   // Overloaded operators have special names
+                                   // and should never be renamed.
+                                   isOverloadedOperator(),
+                                   // Similarly, constructors and destructors
+                                   // should not be considered for renaming.
+                                   cxxConstructorDecl(), cxxDestructorDecl())),
+                      in_blink_namespace));
+  // Note that the matcher for overridden methods doesn't need to filter for
+  // special member functions: see implementation of FunctionDeclRewriter for
+  // the explanation.
+  auto overridden_method_decl_matcher = id("decl", cxxMethodDecl(isOverride()));
+  FunctionDeclRewriter function_decl_rewriter(&replacements);
+  match_finder.addMatcher(function_decl_matcher, &function_decl_rewriter);
+  match_finder.addMatcher(overridden_method_decl_matcher,
+                          &function_decl_rewriter);
+
+  // Call handling:
+  auto call_matcher = id("expr", callExpr(callee(function_decl_matcher)));
+  auto overridden_call_matcher =
+      id("expr", callExpr(callee(overridden_method_decl_matcher)));
+
+  CallRewriter call_rewriter(function_decl_rewriter, &replacements);
+  match_finder.addMatcher(call_matcher, &call_rewriter);
+  match_finder.addMatcher(overridden_call_matcher, &call_rewriter);
+
+  // Function reference handling (e.g. getting a pointer to a function):
   auto function_ref_matcher =
       id("expr", declRefExpr(to(function_decl_matcher)));
-  match_finder.addMatcher(function_ref_matcher, &call_rewriter);
+  // TODO(dcheng): apparently this bit of code isn't exercised in tests.
+  auto overridden_method_ref_matcher =
+      id("expr", declRefExpr(to(overridden_method_decl_matcher)));
+
+  FunctionDeclRefRewriter function_decl_ref_rewriter(function_decl_rewriter,
+                                                     &replacements);
+  match_finder.addMatcher(function_ref_matcher, &function_decl_ref_rewriter);
+  match_finder.addMatcher(overridden_method_ref_matcher,
+                          &function_decl_ref_rewriter);
 
   // Initializer handling:
   auto constructor_initializer_matcher =
       cxxConstructorDecl(forEachConstructorInitializer(
           id("initializer", cxxCtorInitializer(forField(field_decl_matcher)))));
 
-  ConstructorInitializerRewriterCallback constructor_initializer_rewriter(
+  ConstructorInitializerRewriter constructor_initializer_rewriter(
       &replacements);
   match_finder.addMatcher(constructor_initializer_matcher,
                           &constructor_initializer_rewriter);