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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:43
+class LocalRename : public RefactoringAction {
+  StringRef getCommand() const override { return "local-rename"; }
+
----------------
Shouldn't these be public?


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:312
+      [](const std::unique_ptr<RefactoringActionSubcommand> &SubCommand) {
+        return !!(*SubCommand);
+      });
----------------
arphaman wrote:
> ioeric wrote:
> > arphaman wrote:
> > > ioeric wrote:
> > > > Do we need a `FIXME` here?  It seems that this simply finds the first command that is available? What if there are more than one commands that satisfy the requirements?
> > > I don't quite follow, when would multiple subcommands be satisfied? The subcommand corresponds to the action that the user wants to do, there should be no ambiguity.
> > (I added this comment a while ago, and it seemed to have shifted. I was intended to comment on line 297 `  auto It = llvm::find_if(`. Sorry about that.)
> > 
> > I guess I don't quite understand how a specific subcommand is picked based on the commandline options users provide. The `llvm::find_if` above seems to simply find the first action/subcommand that is registered? Could you add a comment about how submamands are matched?
> Ah, I see.
> 
> I added a comment in the updated patch that tries to explain the process. Basically we know that one action maps to just one subcommand because the actions must have unique command names. Since we've already created the subcommands, LLVM's command-line parser enables one particular subcommand that was specified in the command-line arguments. This is what this search does, we just look for the enabled subcommand that was turned on by LLVM's command-line parser, without taking any other command-line arguments into account.
Ohh! I didn't notice `RefactoringActionSubcommand` inherits `cl::SubCommand`. Thanks for the explanation!


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:79
+
+  RefactoringAction &getAction() const { return *Action; }
+
----------------
Why return a mutable reference? Also, this is not used in this patch; maybe leave it out?


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:103
+    IsSelectionParsed = true;
+    // FIXME: Support true selection ranges.
+    StringRef Value = *Selection;
----------------
Is the test selection temporary before we have true selection? 

I am a bit concerned about having test logic in the production code. It makes the code paths a bit complicated, and we might easily end up testing the test logic instead of the actual code logic. I'm fine with this if this is just a short term solution before we have the actual selection support.


================
Comment at: tools/clang-refactor/TestSupport.cpp:187
+Optional<TestSelectionRangesInFile>
+clang_refactor::findTestSelectionRangesIn(StringRef Filename) {
+  ErrorOr<std::unique_ptr<MemoryBuffer>> ErrOrFile =
----------------
I think the `...In` suffix is redundant. We could either get rid of it or use `...InFile`.


Repository:
  rL LLVM

https://reviews.llvm.org/D36574