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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D36574#860203, @klimek wrote:

> In https://reviews.llvm.org/D36574#860197, @arphaman wrote:
>
> > In https://reviews.llvm.org/D36574#858763, @klimek wrote:
> >
> > > One of my main concerns is still that I don't see the need for all the template magic yet :) Why doesn't everybody use the RefactoringResult we define here?
> >
> >
> > @klimek, are you talking about the template usage in this patch or the whole approach in general? I can probably think of some way to reduce the template boilerplate if you are talking about the general approach.
>
>
> Talking about the approach in general :)


I'll work on a patch for a simpler solution as well then. It'll probably be dependent on this one though. I'm pretty sure I need some template magic to figure out how to connect inputs like selection and options and entities that produce source replacements , but I could probably get rid of most of RefactoringActionRule template magic.



================
Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:42
 
+/// This constraint is satisfied when
+template <typename T> struct Decl {
----------------
ioeric wrote:
> when?
Oops, this isn't supposed to be in this patch, I will remove.


================
Comment at: test/Refactor/LocalRename/Field.cpp:4
+class Baz {
+  int /*range=*/Foo; // CHECK: symbol [[@LINE]]:17 -> [[@LINE]]:20
+public:
----------------
klimek wrote:
> arphaman wrote:
> > klimek wrote:
> > > Does this just test the selection?
> > No, this is the moved `clang-rename/Field.cpp` test that tests local-rename. I will move the other tests when this patch is accepted.
> Ok, then I find this test really hard to read - where does it check what the symbol was replaced with?
I thought I should check for occurrences, but you're right, it's probably better to always check the source replacements (we can introduce options that apply non-default occurrences in the future which would allow us to check replacements for occurrences in comments). 

Would something like `//CHECK: "newName" [[@LINE]]:24 -> [[]]:27` work when checking for replacements?


================
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:
> 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.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D36574