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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
