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

[Haojian Wu via Phabricator via cfe-commits]

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