hokein accepted this revision.
hokein added a comment.
LGTM, a few nits.
================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:42
+
+class LocalRename : public RefactoringAction {
+public:
----------------
klimek wrote:
> I have to admit, the implementation here is pretty neat! :)
+1
================
Comment at: test/Refactor/LocalRename/Field.cpp:4
+class Baz {
+ int /*range=*/Foo; // CHECK: =*/Bar;
+public:
----------------
I'd use a completed statement in `CHECK` (like `int /*range=*/Bar;`), which is
more obvious to readers.
================
Comment at: test/Refactor/tool-test-support.c:29
+// CHECK-NEXT: -selection={{.*}}tool-test-support.c:9:29
+
+// CHECK: invoking action 'local-rename':
----------------
Maybe add a comment describing the following cases are `named` test group -- it
took me a while to understand why these "invoking action" messages aren't
ordered by the original source line number.
================
Comment at: tools/clang-refactor/ClangRefactor.cpp:89
+ /// the test file.
+ virtual bool
+ forAllRanges(RefactoringRuleContext &Context,
----------------
nit: `virtual` isn't needed.
================
Comment at: tools/clang-refactor/TestSupport.cpp:28
+using namespace clang;
+using namespace refactor;
+
----------------
I'm not a big fan of `using namespace`. It would make it hard to find out which
namespace is the following method in -- readers have to go to the
corresponding header file to find out the namespace. I'm +1 on using a more
regular way `namespace clang { namespace refactor {...} }`.
================
Comment at: tools/clang-refactor/TestSupport.cpp:127
+
+ virtual void handleError(llvm::Error Err) override {
+ handleResult(std::move(Err));
----------------
nit: virtual can be removed, the same below.
================
Comment at: tools/clang-refactor/TestSupport.h:32
+
+namespace refactor {
+
----------------
Do you plan to use `refactor` on other files?
================
Comment at: tools/clang-refactor/TestSupport.h:104
+
+} // end namespace clang_refactor
+} // end namespace clang
----------------
s/clang_refactor/refactor
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
