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