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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:99
+  template <typename ContextType, typename ValueType, size_t... Is>
+  void invokeImplDispatch(
+      RefactoringResultConsumer &Consumer, RefactoringRuleContext &,
----------------
ioeric wrote:
> Do we still need this overload?
Removed.


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:60
+      : SubCommand(Name, Description), Action(&Action) {
+    Sources = llvm::make_unique<cl::list<std::string>>(
+        cl::Positional, cl::ZeroOrMore, cl::desc("<source0> [... <sourceN>]"),
----------------
ioeric wrote:
> arphaman wrote:
> > arphaman wrote:
> > > ioeric wrote:
> > > > I think you would get a conflict of positional args when you have multiple sub-command instances.
> > > > 
> > > > Any reason not to use `clang::tooling::CommonOptionsParser` which takes care of sources and compilation options for you? 
> > > Not from my experience, I've tried multiple actions it seems like the right arguments are parsed for the right subcommand. It looks like the cl option parser looks is smart enough to handle identical options across multiple subcommands.
> > I agree that using `CommonOptionsParser` would be preferable, but right now it doesn't work well with subcommands. I will create a followup patch that improves subcommand support in `CommonOptionsParser` and uses them in clang-refactor when this patch is accepted.
> Thanks! Could you add a `FIXME` for the `CommonOptionsParser` change?
> 
> 
Done.


================
Comment at: tools/clang-refactor/ClangRefactor.cpp:312
+      [](const std::unique_ptr<RefactoringActionSubcommand> &SubCommand) {
+        return !!(*SubCommand);
+      });
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D36574