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

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

I think this is ready to go.

@klimek Manuel, are all your concerns addressed?



================
Comment at: tools/clang-refactor/ClangRefactor.cpp:62
+  /// \returns true if an error occurred, false otherwise.
+  virtual bool refactorForEachSelection(
+      RefactoringRuleContext &Context,
----------------
arphaman wrote:
> ioeric wrote:
> > 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;
> > ```
> Not sure I fully understand your interface suggestion, but I've tried to decouple some more in the updated patch.
Sorry, I should've been more specific about what I had in mind, but what you have right now is close.  I think this can be further decoupled a bit by having `forAllRanges` take in `SourceManeger` instead of `RefactoringRuleContext` since source selection should only care about source ranges.


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:62
+  virtual void print(raw_ostream &OS) = 0;
+
+  virtual std::unique_ptr<RefactoringResultConsumer> createCustomConsumer() {
----------------
IIUC, `createCustomConsumer` is used to inject testing behavior into clang-refactor. I think this is fine, but please document the intention and the behavior. Thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D36574