[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:29
+using namespace tooling;
+using namespace clang_refactor;
+namespace cl = llvm::cl;
----------------
Sorry for haven't noticed this earlier, but I think `clang::refactor` sounds like a better name space than `clang::clang_refactor`.


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:62
+  /// \returns true if an error occurred, false otherwise.
+  virtual bool refactorForEachSelection(
+      RefactoringRuleContext &Context,
----------------
I would expect the `SourceSelectionArgument` to be more like a container of ranges and further decoupled with the refactoring logic. Same for the `TestSelectionRangesInFile`.

How about an interface like this?
```
template <typename T>
void ForAllRanges(T callback) const;
```


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:64
+      RefactoringRuleContext &Context,
+      llvm::function_ref<Expected<AtomicChanges>(SourceRange R)> Refactor) = 0;
+};
----------------
Do we want to consider other result types?


Repository:
  rL LLVM

https://reviews.llvm.org/D36574