[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: include/clang/Tooling/Refactoring/RefactoringActionRules.h:45
 std::unique_ptr<RefactoringActionRule>
 createRefactoringRule(Expected<ResultType> (*RefactoringFunction)(
                           typename RequirementTypes::OutputType...),
----------------
Can we get rid of this overload and simply make `RefactoringRuleContext` mandatory for all refactoring functions? 


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


================
Comment at: include/clang/Tooling/Refactoring/RefactoringRuleContext.h:44
 
+  ASTContext &getASTContext() const {
+    assert(AST && "no AST!");
----------------
Should we also provide an interface to test if this has an `ASTContext`? Or maybe simply return null if there is `ASTContext`? WDYT?


================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:50
+
+  static Expected<AtomicChanges>
+  renameOccurrences(const RefactoringRuleContext &Context,
----------------
Should this be private?


================
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>]"),
----------------
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?




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


Repository:
  rL LLVM

https://reviews.llvm.org/D36574