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