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