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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
