Refactor Matcher code to reduce per-Matcher boilerplate (#615)

This is in part to eliminate the "sm" and "lang_opts" variables that received comment, but also cuts back on the use of auto.

migrate_cpp/cpp_refactoring/BUILD

@@ -19,7 +19,10 @@ cc_binary(
 cc_library(
     name = "matcher",
     srcs = ["matcher.cpp"],
-    hdrs = ["matcher.h"],
+    hdrs = [
+        "matcher.h",
+        "matcher_manager.h",
+    ],
     deps = [
         "@llvm-project//clang:ast_matchers",
         "@llvm-project//clang:tooling_core",
@@ -29,9 +32,9 @@ cc_library(
 cc_library(
     name = "matcher_test_base",
     testonly = 1,
-    srcs = ["matcher_test_base.cpp"],
     hdrs = ["matcher_test_base.h"],
     deps = [
+        ":matcher",
         "@llvm-project//clang:ast_matchers",
         "@llvm-project//clang:tooling",
         "@llvm-project//llvm:gmock",

migrate_cpp/cpp_refactoring/fn_inserter.cpp

@@ -5,52 +5,42 @@
 #include "migrate_cpp/cpp_refactoring/fn_inserter.h"
 
 #include "clang/ASTMatchers/ASTMatchers.h"
-#include "clang/Lex/Lexer.h"
 
 namespace cam = ::clang::ast_matchers;
 
 namespace Carbon {
 
-FnInserter::FnInserter(std::map<std::string, Replacements>& in_replacements,
-                       cam::MatchFinder* finder)
-    : Matcher(in_replacements) {
-  finder->addMatcher(
-      cam::functionDecl(cam::anyOf(cam::hasTrailingReturn(),
-                                   cam::returns(cam::asString("void"))),
-                        cam::unless(cam::anyOf(cam::cxxConstructorDecl(),
-                                               cam::cxxDestructorDecl())))
-          .bind(Label),
-      this);
-}
-
-void FnInserter::run(const cam::MatchFinder::MatchResult& result) {
-  const auto* decl = result.Nodes.getNodeAs<clang::FunctionDecl>(Label);
-  if (!decl) {
-    llvm::report_fatal_error(std::string("getNodeAs failed for ") + Label);
-  }
+static constexpr char Label[] = "FnInserter";
 
-  auto& sm = *(result.SourceManager);
-  auto lang_opts = result.Context->getLangOpts();
+void FnInserter::Run() {
+  const auto& decl = GetNodeAsOrDie<clang::FunctionDecl>(Label);
 
   // For names like "Class::Method", replace up to "Class" not "Method".
-  clang::NestedNameSpecifierLoc qual_loc = decl->getQualifierLoc();
+  clang::NestedNameSpecifierLoc qual_loc = decl.getQualifierLoc();
   clang::SourceLocation name_begin_loc =
-      qual_loc.hasQualifier() ? qual_loc.getBeginLoc() : decl->getLocation();
+      qual_loc.hasQualifier() ? qual_loc.getBeginLoc() : decl.getLocation();
   auto range =
-      clang::CharSourceRange::getCharRange(decl->getBeginLoc(), name_begin_loc);
+      clang::CharSourceRange::getCharRange(decl.getBeginLoc(), name_begin_loc);
 
   // In order to handle keywords like "virtual" in "virtual auto Foo() -> ...",
   // scan the replaced text and only drop auto/void entries.
   llvm::SmallVector<llvm::StringRef> split;
-  clang::Lexer::getSourceText(range, sm, lang_opts)
-      .split(split, ' ', /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+  GetSourceText(range).split(split, ' ', /*MaxSplit=*/-1, /*KeepEmpty=*/false);
   std::string new_text = "fn ";
   for (llvm::StringRef t : split) {
     if (t != "auto" && t != "void") {
       new_text += t.str() + " ";
     }
   }
-  AddReplacement(*(result.SourceManager), range, new_text);
+  AddReplacement(range, new_text);
+}
+
+auto FnInserterFactory::GetAstMatcher() -> cam::DeclarationMatcher {
+  return cam::functionDecl(cam::anyOf(cam::hasTrailingReturn(),
+                                      cam::returns(cam::asString("void"))),
+                           cam::unless(cam::anyOf(cam::cxxConstructorDecl(),
+                                                  cam::cxxDestructorDecl())))
+      .bind(Label);
 }
 
 }  // namespace Carbon

migrate_cpp/cpp_refactoring/fn_inserter.h

@@ -12,13 +12,13 @@ namespace Carbon {
 // Inserts `fn` for functions and methods.
 class FnInserter : public Matcher {
  public:
-  explicit FnInserter(std::map<std::string, Replacements>& in_replacements,
-                      MatchFinder* finder);
-
-  void run(const MatchFinder::MatchResult& result) override;
+  using Matcher::Matcher;
+  void Run() override;
+};
 
- private:
-  static constexpr char Label[] = "FnInserter";
+class FnInserterFactory : public MatcherFactoryBase<FnInserter> {
+ public:
+  auto GetAstMatcher() -> clang::ast_matchers::DeclarationMatcher override;
 };
 
 }  // namespace Carbon

migrate_cpp/cpp_refactoring/fn_inserter_test.cpp

@@ -9,12 +9,7 @@
 namespace Carbon {
 namespace {
 
-class FnInserterTest : public MatcherTestBase {
- protected:
-  FnInserterTest() : fn_inserter(replacements, &finder) {}
-
-  Carbon::FnInserter fn_inserter;
-};
+class FnInserterTest : public MatcherTestBase<FnInserterFactory> {};
 
 TEST_F(FnInserterTest, TrailingReturn) {
   constexpr char Before[] = "auto A() -> int;";

migrate_cpp/cpp_refactoring/main.cpp

@@ -5,18 +5,18 @@
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Refactoring.h"
 #include "migrate_cpp/cpp_refactoring/fn_inserter.h"
+#include "migrate_cpp/cpp_refactoring/matcher_manager.h"
 #include "migrate_cpp/cpp_refactoring/var_decl.h"
 
-namespace cam = ::clang::ast_matchers;
-namespace ct = ::clang::tooling;
+using clang::tooling::RefactoringTool;
 
 // Initialize the files in replacements. Matcher will restrict replacements to
 // initialized files.
-static void InitReplacements(ct::RefactoringTool* tool) {
-  auto& files = tool->getFiles();
-  auto& repl = tool->getReplacements();
-  for (const auto& path : tool->getSourcePaths()) {
-    auto file = files.getFile(path);
+static void InitReplacements(RefactoringTool* tool) {
+  clang::FileManager& files = tool->getFiles();
+  Carbon::Matcher::ReplacementMap& repl = tool->getReplacements();
+  for (const std::string& path : tool->getSourcePaths()) {
+    llvm::ErrorOr<const clang::FileEntry*> file = files.getFile(path);
     if (file.getError()) {
       llvm::report_fatal_error("Error accessing `" + path +
                                "`: " + file.getError().message() + "\n");
@@ -27,17 +27,16 @@ static void InitReplacements(ct::RefactoringTool* tool) {
 
 auto main(int argc, const char** argv) -> int {
   llvm::cl::OptionCategory category("C++ refactoring options");
-  auto parser = ct::CommonOptionsParser::create(argc, argv, category);
-  ct::RefactoringTool tool(parser->getCompilations(),
-                           parser->getSourcePathList());
+  auto parser =
+      clang::tooling::CommonOptionsParser::create(argc, argv, category);
+  RefactoringTool tool(parser->getCompilations(), parser->getSourcePathList());
   InitReplacements(&tool);
 
   // Set up AST matcher callbacks.
-  auto& repl = tool.getReplacements();
-  cam::MatchFinder finder;
-  Carbon::FnInserter fn_inserter(repl, &finder);
-  Carbon::VarDecl var_decl(repl, &finder);
+  Carbon::MatcherManager matchers(&tool.getReplacements());
+  matchers.Register(std::make_unique<Carbon::FnInserterFactory>());
+  matchers.Register(std::make_unique<Carbon::VarDeclFactory>());
 
   return tool.runAndSave(
-      clang::tooling::newFrontendActionFactory(&finder).get());
+      clang::tooling::newFrontendActionFactory(matchers.GetFinder()).get());
 }

migrate_cpp/cpp_refactoring/matcher.cpp

@@ -4,28 +4,31 @@
 
 #include "migrate_cpp/cpp_refactoring/matcher.h"
 
-namespace ct = ::clang::tooling;
+#include "clang/Basic/SourceManager.h"
 
 namespace Carbon {
 
-void Matcher::AddReplacement(const clang::SourceManager& sm,
-                             clang::CharSourceRange range,
+void Matcher::AddReplacement(clang::CharSourceRange range,
                              llvm::StringRef replacement_text) {
   if (!range.isValid()) {
     // Invalid range.
     return;
   }
-  if (sm.getDecomposedLoc(range.getBegin()).first !=
-      sm.getDecomposedLoc(range.getEnd()).first) {
+  const auto& source_manager = GetSourceManager();
+  if (source_manager.getDecomposedLoc(range.getBegin()).first !=
+      source_manager.getDecomposedLoc(range.getEnd()).first) {
     // Range spans macro expansions.
     return;
   }
-  if (sm.getFileID(range.getBegin()) != sm.getFileID(range.getEnd())) {
+  if (source_manager.getFileID(range.getBegin()) !=
+      source_manager.getFileID(range.getEnd())) {
     // Range spans files.
     return;
   }
 
-  auto rep = ct::Replacement(sm, sm.getExpansionRange(range), replacement_text);
+  auto rep = clang::tooling::Replacement(
+      source_manager, source_manager.getExpansionRange(range),
+      replacement_text);
   auto entry = replacements->find(std::string(rep.getFilePath()));
   if (entry == replacements->end()) {
     // The replacement was in a file which isn't being updated, such as a system
@@ -33,7 +36,7 @@ void Matcher::AddReplacement(const clang::SourceManager& sm,
     return;
   }
 
-  auto err = entry->second.add(rep);
+  llvm::Error err = entry->second.add(rep);
   if (err) {
     llvm::errs() << "Error with replacement `" << rep.toString()
                  << "`: " << llvm::toString(std::move(err)) << "\n";

migrate_cpp/cpp_refactoring/matcher.h

@@ -6,28 +6,87 @@
 #define MIGRATE_CPP_CPP_REFACTORING_MATCHER_H_
 
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
 #include "clang/Tooling/Core/Replacement.h"
 
 namespace Carbon {
 
 // This is an abstract class with helpers to make it easier to write matchers.
-class Matcher : public clang::ast_matchers::MatchFinder::MatchCallback {
+// Note a MatcherFactory (below) is also typically required.
+class Matcher {
  public:
-  // Alias these to elide the namespaces in subclass headers.
-  using MatchFinder = clang::ast_matchers::MatchFinder;
-  using Replacements = clang::tooling::Replacements;
+  using ReplacementMap = std::map<std::string, clang::tooling::Replacements>;
 
-  explicit Matcher(std::map<std::string, Replacements>& in_replacements)
-      : replacements(&in_replacements) {}
+  Matcher(const clang::ast_matchers::MatchFinder::MatchResult* in_match_result,
+          ReplacementMap* in_replacements)
+      : match_result(in_match_result), replacements(in_replacements) {}
+  virtual ~Matcher() = default;
+
+  // Performs main execution of the matcher when a result is found.
+  virtual void Run() = 0;
 
  protected:
   // Replaces the given range with the specified text.
-  void AddReplacement(const clang::SourceManager& sm,
-                      clang::CharSourceRange range,
+  void AddReplacement(clang::CharSourceRange range,
                       llvm::StringRef replacement_text);
 
+  // Returns a matched node by ID, exiting if not present.
+  template <typename NodeType>
+  auto GetNodeAsOrDie(llvm::StringRef id) -> const NodeType& {
+    auto* node = match_result->Nodes.getNodeAs<NodeType>(id);
+    if (!node) {
+      llvm::report_fatal_error(std::string("getNodeAs failed for ") + id);
+    }
+    return *node;
+  }
+
+  // Returns the language options.
+  auto GetLangOpts() -> const clang::LangOptions& {
+    return match_result->Context->getLangOpts();
+  }
+
+  // Returns the full source manager.
+  auto GetSourceManager() -> const clang::SourceManager& {
+    return *match_result->SourceManager;
+  }
+
+  // Returns the source text for a given range.
+  auto GetSourceText(clang::CharSourceRange range) -> llvm::StringRef {
+    return clang::Lexer::getSourceText(range, GetSourceManager(),
+                                       GetLangOpts());
+  }
+
  private:
-  std::map<std::string, Replacements>* const replacements;
+  const clang::ast_matchers::MatchFinder::MatchResult* const match_result;
+  ReplacementMap* const replacements;
+};
+
+// A factory used to instantiate per-MatchResult Matchers, to be registered with
+// the MatcherManager.
+class MatcherFactory {
+ public:
+  virtual ~MatcherFactory() = default;
+
+  virtual auto CreateMatcher(
+      const clang::ast_matchers::MatchFinder::MatchResult* match_result,
+      Matcher::ReplacementMap* replacements) -> std::unique_ptr<Matcher> = 0;
+
+  // Returns the AST matcher which determines when the Matcher is instantiated
+  // and run.
+  virtual auto GetAstMatcher() -> clang::ast_matchers::DeclarationMatcher = 0;
+};
+
+// A convenience factory that implements CreateMatcher for Matchers that have a
+// standard constructor.
+template <typename MatcherType>
+class MatcherFactoryBase : public MatcherFactory {
+ public:
+  auto CreateMatcher(
+      const clang::ast_matchers::MatchFinder::MatchResult* match_result,
+      Matcher::ReplacementMap* replacements)
+      -> std::unique_ptr<Matcher> override {
+    return std::make_unique<MatcherType>(match_result, replacements);
+  }
 };
 
 }  // namespace Carbon

migrate_cpp/cpp_refactoring/matcher_manager.h

@@ -0,0 +1,58 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#ifndef MIGRATE_CPP_CPP_REFACTORING_MATCHER_MANAGER_H_
+#define MIGRATE_CPP_CPP_REFACTORING_MATCHER_MANAGER_H_
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "migrate_cpp/cpp_refactoring/matcher.h"
+
+namespace Carbon {
+
+// Manages registration of AST matchers.
+class MatcherManager {
+ public:
+  explicit MatcherManager(Matcher::ReplacementMap* in_replacements)
+      : replacements(in_replacements) {}
+
+  // Registers Matcher implementations.
+  void Register(std::unique_ptr<MatcherFactory> factory) {
+    matchers.push_back(std::make_unique<MatchCallbackWrapper>(
+        &finder, std::move(factory), replacements));
+  }
+
+  auto GetFinder() -> clang::ast_matchers::MatchFinder* { return &finder; }
+
+ private:
+  // Adapts Matcher for use with MatchCallback.
+  class MatchCallbackWrapper
+      : public clang::ast_matchers::MatchFinder::MatchCallback {
+   public:
+    explicit MatchCallbackWrapper(clang::ast_matchers::MatchFinder* finder,
+                                  std::unique_ptr<MatcherFactory> in_factory,
+                                  Matcher::ReplacementMap* in_replacements)
+        : factory(std::move(in_factory)), replacements(in_replacements) {
+      finder->addMatcher(factory->GetAstMatcher(), this);
+    }
+
+    void run(const clang::ast_matchers::MatchFinder::MatchResult& match_result)
+        override {
+      factory->CreateMatcher(&match_result, replacements)->Run();
+    }
+
+   private:
+    std::unique_ptr<MatcherFactory> factory;
+    Matcher::ReplacementMap* const replacements;
+  };
+
+  Matcher::ReplacementMap* const replacements;
+  clang::ast_matchers::MatchFinder finder;
+  std::vector<std::unique_ptr<MatcherFactory>> factories;
+  std::vector<std::unique_ptr<MatchCallbackWrapper>> matchers;
+};
+
+}  // namespace Carbon
+
+#endif  // MIGRATE_CPP_CPP_REFACTORING_MATCHER_MANAGER_H_

migrate_cpp/cpp_refactoring/matcher_test_base.cpp

@@ -1,39 +0,0 @@
-// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
-// Exceptions. See /LICENSE for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-
-#include "migrate_cpp/cpp_refactoring/matcher_test_base.h"
-
-#include "clang/Tooling/Tooling.h"
-#include "gmock/gmock.h"
-#include "gtest/gtest.h"
-
-namespace ct = ::clang::tooling;
-
-namespace Carbon {
-
-void MatcherTestBase::ExpectReplacement(llvm::StringRef before,
-                                        llvm::StringRef after) {
-  auto factory = ct::newFrontendActionFactory(&finder);
-  constexpr char Filename[] = "test.cc";
-  replacements.clear();
-  replacements.insert({Filename, {}});
-  ASSERT_TRUE(ct::runToolOnCodeWithArgs(
-      factory->create(), before, {}, Filename, "clang-tool",
-      std::make_shared<clang::PCHContainerOperations>(),
-      ct::FileContentMappings()));
-  EXPECT_THAT(replacements, testing::ElementsAre(testing::Key(Filename)));
-  auto actual = ct::applyAllReplacements(before, replacements[Filename]);
-  if (after.find('\n') == std::string::npos) {
-    EXPECT_THAT(*actual, testing::Eq(after.str()));
-  } else {
-    // Split lines to get gmock to get an easier-to-read error.
-    llvm::SmallVector<llvm::StringRef, 0> actual_lines;
-    llvm::SplitString(*actual, actual_lines, "\n");
-    llvm::SmallVector<llvm::StringRef, 0> after_lines;
-    llvm::SplitString(after, after_lines, "\n");
-    EXPECT_THAT(actual_lines, testing::ContainerEq(after_lines));
-  }
-}
-
-}  // namespace Carbon

migrate_cpp/cpp_refactoring/matcher_test_base.h

@@ -7,19 +7,57 @@
 
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Tooling/Core/Replacement.h"
+#include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include "migrate_cpp/cpp_refactoring/matcher_manager.h"
 
 namespace Carbon {
 
+// Matcher test framework.
+template <typename MatcherFactoryType>
 class MatcherTestBase : public ::testing::Test {
  protected:
+  MatcherTestBase() : matchers(&replacements) {
+    matchers.Register(std::make_unique<MatcherFactoryType>());
+  }
+
   // Expects that that the replacements produced by running the finder result in
   // the specified code transformation.
-  void ExpectReplacement(llvm::StringRef before, llvm::StringRef after);
+  void ExpectReplacement(llvm::StringRef before, llvm::StringRef after) {
+    auto factory =
+        clang::tooling::newFrontendActionFactory(matchers.GetFinder());
+    constexpr char Filename[] = "test.cc";
+    replacements.clear();
+    replacements.insert({Filename, {}});
+    ASSERT_TRUE(clang::tooling::runToolOnCodeWithArgs(
+        factory->create(), before, {}, Filename, "clang-tool",
+        std::make_shared<clang::PCHContainerOperations>(),
+        clang::tooling::FileContentMappings()));
+    EXPECT_THAT(replacements, testing::ElementsAre(testing::Key(Filename)));
+    llvm::Expected<std::string> actual =
+        clang::tooling::applyAllReplacements(before, replacements[Filename]);
+
+    // Make a specific note if the matcher didn't make any changes.
+    std::string unchanged;
+    if (before == *actual) {
+      unchanged = "NOTE: Actual matches original text, no changes made.";
+    }
+
+    if (after.find('\n') == std::string::npos) {
+      EXPECT_THAT(*actual, testing::Eq(after.str())) << unchanged;
+    } else {
+      // Split lines to get gmock to get an easier-to-read error.
+      llvm::SmallVector<llvm::StringRef, 0> actual_lines;
+      llvm::SplitString(*actual, actual_lines, "\n");
+      llvm::SmallVector<llvm::StringRef, 0> after_lines;
+      llvm::SplitString(after, after_lines, "\n");
+      EXPECT_THAT(actual_lines, testing::ContainerEq(after_lines)) << unchanged;
+    }
+  }
 
-  std::map<std::string, clang::tooling::Replacements> replacements;
-  clang::ast_matchers::MatchFinder finder;
+  Matcher::ReplacementMap replacements;
+  MatcherManager matchers;
 };
 
 }  // namespace Carbon

migrate_cpp/cpp_refactoring/var_decl.cpp

@@ -5,21 +5,12 @@
 #include "migrate_cpp/cpp_refactoring/var_decl.h"
 
 #include "clang/ASTMatchers/ASTMatchers.h"
-#include "clang/Lex/Lexer.h"
 
 namespace cam = ::clang::ast_matchers;
 
 namespace Carbon {
 
-VarDecl::VarDecl(std::map<std::string, Replacements>& in_replacements,
-                 cam::MatchFinder* finder)
-    : Matcher(in_replacements) {
-  // Skip range-based for loops in this var-inserter.
-  finder->addMatcher(cam::varDecl(cam::unless(cam::hasParent(cam::declStmt(
-                                      cam::hasParent(cam::cxxForRangeStmt())))))
-                         .bind(Label),
-                     this);
-}
+static constexpr char Label[] = "VarDecl";
 
 // Helper function for printing TypeLocClass. Useful for debugging.
 LLVM_ATTRIBUTE_UNUSED
@@ -38,12 +29,10 @@ static auto TypeLocClassToString(clang::TypeLoc::TypeLocClass c)
 }
 
 // Returns a string for the type.
-static auto GetTypeStr(const clang::VarDecl* decl,
-                       const clang::SourceManager& sm,
-                       const clang::LangOptions& lang_opts) -> std::string {
+auto VarDecl::GetTypeStr(const clang::VarDecl& decl) -> std::string {
   // Built a vector of class information, because we'll be traversing reverse
   // order to construct the final type.
-  auto type_loc = decl->getTypeSourceInfo()->getTypeLoc();
+  auto type_loc = decl.getTypeSourceInfo()->getTypeLoc();
   std::vector<std::pair<clang::TypeLoc::TypeLocClass, std::string>> segments;
   while (!type_loc.isNull()) {
     std::string text;
@@ -54,8 +43,7 @@ static auto GetTypeStr(const clang::VarDecl* decl,
     }
     auto range =
         clang::CharSourceRange::getTokenRange(type_loc.getLocalSourceRange());
-    std::string range_str =
-        clang::Lexer::getSourceText(range, sm, lang_opts).str();
+    std::string range_str = GetSourceText(range).str();
 
     // Make a list of segments with their TypeLocClass for reconstruction of the
     // string. Locally, we will have a qualifier (such as `const`) and a type
@@ -101,46 +89,44 @@ static auto GetTypeStr(const clang::VarDecl* decl,
   return type_str;
 }
 
-void VarDecl::run(const cam::MatchFinder::MatchResult& result) {
-  const auto* decl = result.Nodes.getNodeAs<clang::VarDecl>(Label);
-  if (!decl) {
-    llvm::report_fatal_error(std::string("getNodeAs failed for ") + Label);
-  }
-
-  if (decl->getTypeSourceInfo() == nullptr) {
+void VarDecl::Run() {
+  const auto& decl = GetNodeAsOrDie<clang::VarDecl>(Label);
+  if (decl.getTypeSourceInfo() == nullptr) {
     // TODO: Need to understand what's happening in this case. Not sure if we
     // need to address it.
     return;
   }
 
-  auto& sm = *(result.SourceManager);
-  auto lang_opts = result.Context->getLangOpts();
-
   std::string after;
-  if (decl->getType().isConstQualified()) {
+  if (decl.getType().isConstQualified()) {
     after = "let ";
-  } else if (result.Nodes.getNodeAs<clang::ParmVarDecl>(Label) == nullptr) {
+  } else if (!llvm::isa<clang::ParmVarDecl>(&decl)) {
     // Start the replacement with "var" unless it's a parameter.
     after = "var ";
   }
   // Add "identifier: type" to the replacement.
-  after += decl->getNameAsString() + ": " + GetTypeStr(decl, sm, lang_opts);
+  after += decl.getNameAsString() + ": " + GetTypeStr(decl);
 
   // This decides the range to replace. Normally the entire decl is replaced,
   // but for code like `int i, j` we need to detect the comma between the
   // declared names. That case currently results in `var i: int, var j: int`.
   // If there's a comma, this range will be non-empty.
-  auto type_loc = decl->getTypeSourceInfo()->getTypeLoc();
-  auto after_type_loc =
-      clang::Lexer::getLocForEndOfToken(type_loc.getEndLoc(), 0, sm, lang_opts);
-  auto comma_source_text = clang::Lexer::getSourceText(
-      clang::CharSourceRange::getCharRange(after_type_loc, decl->getLocation()),
-      sm, lang_opts);
+  auto type_loc = decl.getTypeSourceInfo()->getTypeLoc();
+  clang::SourceLocation after_type_loc = clang::Lexer::getLocForEndOfToken(
+      type_loc.getEndLoc(), 0, GetSourceManager(), GetLangOpts());
+  llvm::StringRef comma_source_text = GetSourceText(
+      clang::CharSourceRange::getCharRange(after_type_loc, decl.getLocation()));
   bool has_comma = !comma_source_text.trim().empty();
   clang::CharSourceRange replace_range = clang::CharSourceRange::getTokenRange(
-      has_comma ? decl->getLocation() : decl->getBeginLoc(), decl->getEndLoc());
+      has_comma ? decl.getLocation() : decl.getBeginLoc(), decl.getEndLoc());
+
+  AddReplacement(replace_range, after);
+}
 
-  AddReplacement(sm, replace_range, after);
+auto VarDeclFactory::GetAstMatcher() -> cam::DeclarationMatcher {
+  return cam::varDecl(cam::unless(cam::hasParent(cam::declStmt(
+                          cam::hasParent(cam::cxxForRangeStmt())))))
+      .bind(Label);
 }
 
 }  // namespace Carbon

migrate_cpp/cpp_refactoring/var_decl.h

@@ -12,13 +12,16 @@ namespace Carbon {
 // Updates variable declarations for `var name: Type`.
 class VarDecl : public Matcher {
  public:
-  explicit VarDecl(std::map<std::string, Replacements>& in_replacements,
-                   MatchFinder* finder);
-
-  void run(const MatchFinder::MatchResult& result) override;
+  using Matcher::Matcher;
+  void Run() override;
 
  private:
-  static constexpr char Label[] = "VarDecl";
+  auto GetTypeStr(const clang::VarDecl& decl) -> std::string;
+};
+
+class VarDeclFactory : public MatcherFactoryBase<VarDecl> {
+ public:
+  auto GetAstMatcher() -> clang::ast_matchers::DeclarationMatcher override;
 };
 
 }  // namespace Carbon

migrate_cpp/cpp_refactoring/var_decl_test.cpp

@@ -9,12 +9,7 @@
 namespace Carbon {
 namespace {
 
-class VarDeclTest : public MatcherTestBase {
- protected:
-  VarDeclTest() : var_decl(replacements, &finder) {}
-
-  Carbon::VarDecl var_decl;
-};
+class VarDeclTest : public MatcherTestBase<VarDeclFactory> {};
 
 TEST_F(VarDeclTest, Declaration) {
   constexpr char Before[] = "int i;";