clang-rename: split existing options into two new subcommands

[vmiklos]

Summary

• rename-at is meant to be integrated with editors and works mainly off of a location in a file, and this is the default
• rename-all is optimized for one or more oldname->newname renames, and works with clang-apply-replacements

[vmiklos]

Manuel, do you have an opinion on this, please?

[klimek]

I think we really want 2 tools:
a) one that is optimized for oldname->newname renames, and supports the multi-TU case really well
b) one that is meant to be integrated with editors and works mainly off of a location in a file

I'm a bit torn whether putting those 2 into the same executable is a good idea. Looping in Benjamin for additional ideas.

[bkramer]

In D21814#475322, @klimek wrote:

I think we really want 2 tools:
a) one that is optimized for oldname->newname renames, and supports the multi-TU case really well
b) one that is meant to be integrated with editors and works mainly off of a location in a file

I'm a bit torn whether putting those 2 into the same executable is a good idea. Looping in Benjamin for additional ideas.

I'm fine with both things living in the same binary. The location thing is just a different way of specifying the symbol name. I fail to see how that's related to this review though.

[klimek]

In D21814#476572, @bkramer wrote:

In D21814#475322, @klimek wrote:

I think we really want 2 tools:
a) one that is optimized for oldname->newname renames, and supports the multi-TU case really well
b) one that is meant to be integrated with editors and works mainly off of a location in a file

I'm a bit torn whether putting those 2 into the same executable is a good idea. Looping in Benjamin for additional ideas.

I'm fine with both things living in the same binary. The location thing is just a different way of specifying the symbol name. I fail to see how that's related to this review though.

Well, it's about how much features we want to pack into the same binary; once we basically have multiple tools in one tool, adding more different use cases makes sense, I think.

[omtcyf0]

Hi @vmiklos!

Thank you very much for contributing to clang-rename.

The patch looks nice, but it conflicts with my understanding of the view on what the tool should do.

Generally, I do not support the idea of adding an option to perform multiple renamings at once. Here are my reasons:

• I think the tool should mainly target editors. Most users won't use command line interface IMHO. Thus, performing one renaming action at once seems more than logical to me, because you don't do multiple (at once) in the editor.

• There's a patch I wish to put for review soon, which is about checking -new-name - whether it is a valid identifier or not. This will be the first step towards teaching the tool to understand whether the change will break the codebase or not (i.e. there will be name conflict, invalid identifier or something else). So, we'll have to check each "new-name" for being valid, check each "old-name" for being valid, check whether we have the same number of "new-name"s and "old-name"s (or offsets) [which you have in your patch], then for each pair check whether the renaming action would be OK.

• With this patch we're bringing tokenization of these names into ClangRename.cpp, which doesn't seem cool to me. I think the main file is supposed to be tool-logic only, not sure if putting much logic-unrelated code is good there.

To sum up, my main points are:

• I think we should try to make a good interface, which can be easily reused by the editors instead of trying to target CLI. You can try very simple vim integration, see latest http://reviews.llvm.org/D22087.

• I believe the tool shouldn't try to do literally everything and get features, which require much hardcoding and cornercases handling. Doing only one exact thing and doing it very good seems reasonable to me.

Anyway, if everyone is happy about moving in this direction aswell, I'm fine too, but these are just my thoughts.

If you want to chat, feel free to drop me an email :)

[vmiklos]

Kirill: OK, so you're in the camp marked as b) by Manuel. Sure, the vim integration is nice (I'm also a vim user), now that you mentioned it, I need to go and try it myself. ;-) Given the above patch, probably it's obvious that I'm more in camp a). I don't insist on having that in a tool named clang-rename, though.

Hmm, so here is the summary of the comments so far, as far as I understand:

• Manuel: it's OK to handle multiple renames with a single command-line invocation, but not sure if clang-rename should do that, or if it should be a separate tool
• Benjamin: no problem with clang-rename having this feature
• Kirill: clang-rename should definitely not have this feature.

What's the consensus, should I rework this patch, so that it doesn't touch tool/ClangRename.cpp, but adds a new tool (named e.g. clang-multi-rename, still linking to clangRename)? I can do that, but it would be good to hear first if it is worth the effort, or there would be still "the patch is correct, but it's not the way to go" style comments. :-)

Thanks.

[omtcyf0]

Miklos, thanks for the feedback!

Hm, I'm not sure about a) and b) camps here. I think we can have both. It may be that I haven't looked too much into the code or I am missing something, but so far both integration and cross-TU analysis seem OK together in one tool as for me.

So far, integration (in it's very very simple way) is very straightforward: we just have a binary and we call it via passing two parameters (offset and new-name). This doesn't require neither changes in the tool as it is nor any difficult tweaks.

As for cross-TU stuff, why can't it be here, too? We'll still need it at some point.

Creating clang-multi-rename doesn't seem right at all - there are already too many tools which some users don't understand. If the command line interface with this feature is a useful thing to have, then we should definitely just add it here. One issue I see is that I think it won't be useful to most users, but a) I can be wrong b) It's not the main one :)

Probably my main point is: let's not bring too much features into the interface of the yet non-correctly working tool. It feels right to me to ensure first that the tool actually works correctly. Because it does not at the moment (see http://reviews.llvm.org/D22102 XFAIL test for example).

After we do that, we will have the feedback from the community, because they will try the first version, which actually works. We might get at least few people just trying it at least and saying whether for example it's not usable at all in the editor and that they would like an extended CLI. I'll be all for the changes similar to these proposed by you then!

In my opinion we should (right now) only support a single use scenario: clang-rename -offset=[OFFSET] -new-name=[NAME]. Not only because we would keep things easy (which I'd honestly be very happy about, but, I know, people hate me for that :D), but because when we get our first users, chances of them finding some configuration of parameters which breaks than simples single scenario is very high, not talking about the multiple scenarios you're proposing. Why would we need many ways to interact with an incorrect tool? We should probably have a single way to interact with a completely correct tool.

At the end I think we can both be happy if we separate the CLI and the renaming interface completely. The editors can interact with a C++ Library then and clang-rename would become a (probably) sophisticated command line interface to that library, only dealing with user-library interaction thingie. We can't have both at the moment, of that I am certain.

My proposal is to:

• Improve Vim interface just a little, I think I can do that tomorrow.
• Write a letter to the cfe-dev, explaining that we want clang-rename to become useful and that we want their feedback. I'll send it tomorrow, right after I'll be done with the Vim interface slight improvements. We'll probably get more interesting ideas and, most importantly, at least few first users, who will help us to understand what they need. Discussions on reviews get lost and forgotten, cfe-dev is always there and living.
• Get a proper test set for clang-rename. Really. Because now it's not tested at all. Fill in bugs (hopefully the guys who will try it at least will help), get suggestions, get opinions. Probably make a list/plan of them.
• Address the issues and then think of the additional stuff we can add (like separating the interface and routine and all that).

As I'm going to invest very much time into the project I hope we can do that!

I'll be very happy to hear more opinions from the others both here and in the cfe-dev list after I send the message!

@klimek, @bkramer, what do you think of this?

[klimek]

Kirill, I think it's important to note that Miklos is probably a user, otherwise I'd guess he wouldn't add those features.
Generally, we also internally have a (large scale) simplified rename tool that allows renaming multiple things in a code base at once; it is definitely a useful tool to have.
I believe that Benjamin expresses the opinion of multiple people in the community (usually including Chandler) who want fewer binaries.
Perhaps we can make this better by doing the following compromise:
Instead of just having tons of parameters where it's hard to say which parameter does what, have 2 different commands that you can call, each with their own set of flags:
clang-rename rename-at -offset ... -newname ...
clang-rename rename-all -oldname ... -newname ... -oldname ... -new-name ... (or similar)

[omtcyf0]

Manuel,

I think it's important to note that Miklos is probably a user, otherwise I'd guess he wouldn't add those features.

Sure. But there are different kinds of users :) IIRC there's a discussion in cfe-def called "Clang should natively support Fortran" or something. Not saying it's bad, but needs of different users may seem strange to the others.

Generally, we also internally have a (large scale) simplified rename tool that allows renaming multiple things in a code base at once; it is definitely a useful tool to have.

Hm, I see. So if that's an often used scenario I'm all for it :)

Perhaps we can make this better by doing the following compromise:
Instead of just having tons of parameters where it's hard to say which parameter does what, have 2 different commands that you can call, each with their own set of flags:
clang-rename rename-at -offset ... -newname ...
clang-rename rename-all -oldname ... -newname ... -oldname ... -new-name ... (or similar)

This is probably similar to:

At the end I think we can both be happy if we separate the CLI and the renaming interface completely. The editors can interact with a C++ Library then and clang-rename would become a command line interface with many options to that library

So I guess I can probably get to that if this seems reasonable to you. I'd be happy to have a pure and clear interface, which won't deal with wrong user input etc.

Thanks for the feedback!

[vmiklos]

As far as I see tooling::CommonOptionsParser (in its current form) does not handle cl::SubCommand instances. Should I fix that or would it be OK to switch to using cl::ParseCommandLineOptions directly in clang-rename?

[klimek]

In D21814#478879, @vmiklos wrote:

As far as I see tooling::CommonOptionsParser (in its current form) does not handle cl::SubCommand instances. Should I fix that or would it be OK to switch to using cl::ParseCommandLineOptions directly in clang-rename?

Well, if you switch, you'll lose a lot of what CommonOptionsParser gives you... So I think it's useful to keep it working with CommonOptionsParser.

[vmiklos]

The alternative is to change the CommonOptionsParser ctor to take a vector of OptionCategory&, and in that case we can add the -extra-arg-before and other options to all subcommands, but that means he'll have to adapt all client code, i.e. all tools in clang-tools-extra that uses CommonOptionsParser. Is that an acceptable cost? Or did I miss some easier way?

[klimek]

In D21814#485410, @vmiklos wrote:

The alternative is to change the CommonOptionsParser ctor to take a vector of OptionCategory&, and in that case we can add the -extra-arg-before and other options to all subcommands, but that means he'll have to adapt all client code, i.e. all tools in clang-tools-extra that uses CommonOptionsParser. Is that an acceptable cost? Or did I miss some easier way?

So Subcommand doesn't support having a set of common options?

[vmiklos]

I'm a bit confused.

On one hand, I want to use tooling::CommonOptionsParser to parse the options, which needs a cl::OptionCategory as a parameter.

On the other hand, I want to parse the options, based on that I'll know what subcommand was requested, and then I can choose the right option category. Based on the above example, I want to use an option category that recognizes -oldname when "rename-all" is used, and I want an other option category that recognizes -offset" when "rename-at" is used.

I sense a chicken-and-egg problem, the argument of CommonOptionsParser needs the category, but I'll only know the correct category after I called CommonOptionsParser. Or is there a way out of this? ;-)

[vmiklos]

I've implemented the requested split of options, now there are two new rename-at and rename-all subcommands. The only common options is -i and -new-name, nothing else is shared, apart from the common options added by tooling::CommonOptionsParser. The code is modeled after llvm-cov, that way I could break the chicken-and-egg problem I outlined in my previous comment.

I've also updated all tests to use either of the two subcommands.

For now I did not touch documentation, though that'll be obviously the next step, I just didn't want to make this patch larger than necessary. :-)

[omtcyfz]

• Can you please update diff? I changed most of the tests recently.
• I think you should update doc/clang-rename in this patch (making it a subsequent patch isn't worthy IMO)
• Updating clang-rename/tool/clang-rename.py (simply add -rename-at into the argument list there) seems reasonable.
• Also, I'd be happy to see at least few good tests for -rename-all with multiple -old-name and -new-name arguments.
• Why does -rename-at not have -export-fixes option anymore?
• Is there really a need to dispatch main to renameAtMain and renameAllMain? Most of the code is exactly the same (apart from YAML dump absence in renameAtMain, which I do not understand).

[vmiklos]

In D21814#486204, @omtcyfz wrote:

• Can you please update diff? I changed most of the tests recently.

Sure, I actually wanted to ask if those test additions were meant to be test
renames. :-)

• I think you should update doc/clang-rename in this patch (making it a subsequent patch isn't worthy IMO)

Done.

• Updating clang-rename/tool/clang-rename.py (simply add -rename-at into the argument list there) seems reasonable.

Done.

• Also, I'd be happy to see at least few good tests for -rename-all with multiple -old-name and -new-name arguments.

Multiple -old-name / -new-name is not supported yet. I implemented that in the
first diff of this review, but then I was asked to split the two use cases into
separate subcommands first, and only support the multi-rename feature in
rename-all only. So I plan to add that in a subsequent patch. Or should squash
even that into this review?

• Why does -rename-at not have -export-fixes option anymore?

The use-case for -export-fixes was that multiple translation units will want to
do the same replacements in common headers, so -i is not a good choice there.
Instead using -export-fixes, and then letting clang-apply-replacements do the
deduplication is the way to go. From this point of view, -export-fixes is not
useful for the rename-at / single TU use-case. But no problem, I've added it
back.

• Is there really a need to dispatch main to renameAtMain and renameAllMain? Most of the code is exactly the same (apart from YAML dump absence in renameAtMain, which I do not understand).

The first idea was to use two separate binaries for rename-at/rename-all. Then
a compromise was to still have the same binary, but separate subcommands. So I
thought it's considered good to have a separate implementation of the separate
subcommands. But I'm happy if sharing code between rename-at and rename-all is
still OK, I've changed that.

[omtcyfz]

In D21814#486269, @vmiklos wrote:

In D21814#486204, @omtcyfz wrote:

• Can you please update diff? I changed most of the tests recently.

Sure, I actually wanted to ask if those test additions were meant to be test
renames. :-)

Yeah, sorry for that...

• I think you should update doc/clang-rename in this patch (making it a subsequent patch isn't worthy IMO)

Done.

• Updating clang-rename/tool/clang-rename.py (simply add -rename-at into the argument list there) seems reasonable.

Done.

• Also, I'd be happy to see at least few good tests for -rename-all with multiple -old-name and -new-name arguments.

Multiple -old-name / -new-name is not supported yet. I implemented that in the
first diff of this review, but then I was asked to split the two use cases into
separate subcommands first, and only support the multi-rename feature in
rename-all only. So I plan to add that in a subsequent patch. Or should squash
even that into this review?

Well, it might be fine for this one. Let's see what the others have to say.

• Why does -rename-at not have -export-fixes option anymore?

The use-case for -export-fixes was that multiple translation units will want to
do the same replacements in common headers, so -i is not a good choice there.
Instead using -export-fixes, and then letting clang-apply-replacements do the
deduplication is the way to go. From this point of view, -export-fixes is not
useful for the rename-at / single TU use-case. But no problem, I've added it
back.

Ah, I can see your point. Well, there's still a long long way to the multi-TU stuff anyway... But I hope we'll get there at some point. I think both interfaces might be useful for multi-TU swell.

• Is there really a need to dispatch main to renameAtMain and renameAllMain? Most of the code is exactly the same (apart from YAML dump absence in renameAtMain, which I do not understand).

The first idea was to use two separate binaries for rename-at/rename-all. Then
a compromise was to still have the same binary, but separate subcommands. So I
thought it's considered good to have a separate implementation of the separate
subcommands. But I'm happy if sharing code between rename-at and rename-all is
still OK, I've changed that.

Hm, I didn't think about it.

Well, honestly I'm not a fan of getting too many binaries and at the moment I think both interfaces are almost identical, so ATM I don't think we should get second binary, it will just make things even more complicated.

[vmiklos]

Is there anything I can help with to get this reviewed, please? As far as I see it still applies cleanly on top of current trunk.

[klimek]

Kirill, you happy with this approach?

[omtcyfz]

The patch looks fine to me (though I'm not sure if there are no new tests; if they are interface changes should be applied).

If everyone seems to be in favor of such changes, I'm OK with it, but in general I think it makes things more complicated and I'm not sure if it's necessary at the moment; I expressed my ideas about it in comments to the other patch. But if that's what the common use-case is... So, TL;DR I personally don't see why one would want to rename multiple things at once while we still can't rename a single symbol correctly in too many cases...

P.S. it seems logical to me to support -offset option in -rename-all, too. And introducing -rename-all without actually supporting multiple renaming actions "at once" seems weird to me, too.

clang-rename/tool/ClangRename.cpp

226 ↗

(On Diff #64231)

use std::function here?

[vmiklos]

The patch looks fine to me (though I'm not sure if there are no new tests; if
they are interface changes should be applied).

make check-clang-tools + the patch at r276098 passes for me at least. But any
pending test should be trivial to adapt.

P.S. it seems logical to me to support -offset option in -rename-all,
too.

OK, I've added that, with a testcase.

And introducing -rename-all without actually supporting multiple
renaming actions "at once" seems weird to me, too.

OK, I've squashed the original diff into this one, with a testcase, which
addresses this.

A nice side-effect is that now the option parser takes care of checking if
-new-name is provided, no need to have explicit code for that in clang-rename.

use std::function here?

Done, also changed the typedef to using.

[vmiklos]

Rebased on top of r276282 and resolved conflicts.

[klimek]

clang-rename/RenamingAction.cpp

48 ↗

(On Diff #64941)

Question is whether if we go down that route we shouldn't do one search for all the names, instead of re-looping for each name.

[vmiklos]

clang-rename/RenamingAction.cpp

48 ↗

(On Diff #64941)

You mean improving USRLocFindingASTVisitor, so that it can work with a list of USRs, not with just a single USR? I can do that, but if possible, I would like to do that as a follow-up patch; it was called in a loop already before this patch, so sounds orthogonal. (And it would potentially conflict with any pending patches that touch that class.)

I could fold HandleOneRename() into HandleTranslationUnit(), but that just makes the resulting function larger, and does not get rid of the looping. Or is that what you want?

[klimek]

Kirill, unless you have *specific* issues with this patch, I think it's good to land.

clang-rename/RenamingAction.cpp

48 ↗

(On Diff #64941)

No, the former. Can you add a FIXME?

[omtcyfz]

I'd be actually happy if instead of having -rename-at option we'd have this behavior by default unless -rename-all is used.

[vmiklos]

In D21814#492540, @omtcyfz wrote:

I'd be actually happy if instead of having -rename-at option we'd have this behavior by default unless -rename-all is used.

Not sure I understand this request. rename-at and rename-all all subcommands, not options. "have this by default", you mean the ability to perform multiple oldname->newname replacements with one commandline invocation? If so, not allowing that when the rename-all subcommand is chosen sounds confusing to me. But perhaps I misunderstand something. ;-)

I can make the rename-at subcommand optional, and when not specifying a subcommand, assume rename-at was specified (unless -help or -version is used). Is this what you want?

[omtcyfz]

I can make the rename-at subcommand optional, and when not specifying a subcommand, assume rename-at was specified (unless -help or -version is used). Is this what you want?

Yep, exactly.

Sorry, I might have not expressed my idea good enough.

[vmiklos]

Done, that also allows not modifying most existing tests.

[vmiklos]

Rebased on top of r276684 and resolved conflicts.

[klimek]

Kirill, are your specific concerns addressed?

[vmiklos]

Rebased on top of r276836 and resolved conflicts.

[omtcyfz]

Apart from my high level concerns, which of course I still have...

clang-rename/tool/clang-rename.py

43 ↗

(On Diff #65672)

rename-at isn't necessary here anymore since it's going to be default behavior IIUC

docs/clang-rename.rst

35 ↗

(On Diff #65672)

docs should be fixed correspondingly; i.e. prefer to write clang-rename -offset=42.... over clang-rename -rename-at

50 ↗

(On Diff #65672)

here and later, too

[vmiklos]

rename-at isn't necessary here anymore since it's going to be default behavior IIUC

Indeed, it can be changed back now, done.

docs should be fixed correspondingly; i.e. prefer to write clang-rename -offset=42.... over clang-rename -rename-at

Done.

here and later, too

clang-rename rename-at is now only mentioned by the document when it comes to
clang-rename rename-at -help, which is still necessary, as `clang-rename
-help` talks about the subcommands only. I've updated clang-rename -help so
that it points out that rename-at is the default, though.

[vmiklos]

Rebased on top of r276949 and resolved a failing test (FunctionWithClassFindByName.cpp).

[vmiklos]

Is there anything I can help with to get this accepted, please? As far as I see I addressed all so far mentioned concerns. Thanks.

[omtcyfz]

1. Run clang-format or something, 80 char width limit is broken in tool/ClangRename.cpp dozen of times.
2. Only do outs() << "abcd\n" << "efgh\n" if you have something in between, which can not be predefined. I.e. if you have

/// \brief Top level help.
static int helpMain(int argc, const char *argv[]) {
  errs() << "Usage: clang-rename {rename-at|rename-all} [OPTION]...\n\n"
         << "A tool to rename symbols in C/C++ code.\n\n"
         << "Subcommands:\n"
         << "  rename-at:  Perform rename off of a location in a file. (This is the default.)\n"
         << "  rename-all: Perform rename of all symbols matching one or more fully qualified names.\n";
  return 0;
}

Just make it a const string, isn't it better?

3. tooling::RefactoringTool takes care of printing version IIUC, no need to do that in a custom way (we don't have custom version in clang-rename head at the moment, that was something @Eugene.Zelenko sent recently).
4. clang-rename/RenamingAction.h -> USRList -> USRs or the other way around everywhere. So far single naming convention feels right to me (I personally prefer *s over *List). LLVM Coding Style does, too, from what I understand. Unless it's *Set, which is pretty much different thing.
5. Do we really need to dispatch these functions this way? with

enum RenameSubcommand {
  RenameAt, RenameAll
};

And many other strange things. Just pass bool isMultipleRename or isRenameAll to main routine instead of creating enum. We only have two such options here, right? I pray we won't have more.

6. Move int main(int argc, const char **argv) upwards, so that it's above dispatched routines. Otherwise when one wants to see what's going on, he sees subroutine first without understanding where it comes from. Other way around feels more intuitive to me.

Feel free do disagree with my points, I may be terribly wrong :)

[vmiklos]

1. Run clang-format or something, 80 char width limit is broken in tool/ClangRename.cpp dozen of times.

Done. I was afraid doing that, due to the changes not related to my patch, but
the result doesn't seem to be too bad. I guess in a later patch it would be
great to run clang-format on the whole clang-rename code, then all contributors
could just run clang-format before committing in case LLVM coding style is not
in our muscle memory. :)

2. Only do outs() << "abcd\n" << "efgh\n" if you have something in between, which can not be predefined. I.e. if you have

/// \brief Top level help.
static int helpMain(int argc, const char *argv[]) {
  errs() << "Usage: clang-rename {rename-at|rename-all} [OPTION]...\n\n"
         << "A tool to rename symbols in C/C++ code.\n\n"
         << "Subcommands:\n"
         << "  rename-at:  Perform rename off of a location in a file. (This is the default.)\n"
         << "  rename-all: Perform rename of all symbols matching one or more fully qualified names.\n";
  return 0;
}

Just make it a const string, isn't it better?

Done. I copied llvm-cov, but I have no problems changing it.

3. tooling::RefactoringTool takes care of printing version IIUC, no need to do that in a custom way (we don't have custom version in clang-rename head at the moment, that was something @Eugene.Zelenko sent recently).

Indeed, removed.

4. clang-rename/RenamingAction.h -> USRList -> USRs or the other way around everywhere. So far single naming convention feels right to me (I personally prefer *s over *List). LLVM Coding Style does, too, from what I understand. Unless it's *Set, which is pretty much different thing.

I've changed NewNameList and PrevNameList.

USRList refers to a list of lists, i.e. doing one oldname->newname rename may
have to deal with multiple USRs, and when doing multiple oldname->newname
renames, you need to deal with a list of list of USRs. I used the List suffix
in this "list of list" case. I have no problem renaming
std::vector<std::vector<std::string>> USRList to USRs, but then we need a new
name for std::vector<std::string> USRs.

5. Do we really need to dispatch these functions this way? with

enum RenameSubcommand {
  RenameAt, RenameAll
};

And many other strange things. Just pass bool isMultipleRename or isRenameAll to main routine instead of creating enum. We only have two such options here, right? I pray we won't have more.

I promise I don't plan to suggest more. :) Changed the enum to a bool.

6. Move int main(int argc, const char **argv) upwards, so that it's above

dispatched routines. Otherwise when one wants to see what's going on, he sees
subroutine first without understanding where it comes from. Other way around
feels more intuitive to me.

Done.

[omtcyfz]

In D21814#500481, @vmiklos wrote:

1. Run clang-format or something, 80 char width limit is broken in tool/ClangRename.cpp dozen of times.

Done. I was afraid doing that, due to the changes not related to my patch, but
the result doesn't seem to be too bad. I guess in a later patch it would be
great to run clang-format on the whole clang-rename code, then all contributors
could just run clang-format before committing in case LLVM coding style is not
in our muscle memory. :)

Yes, but I don't think there are many locations where clang-format would be triggered. I believe I ran clang-format over most parts few times.

2. Only do outs() << "abcd\n" << "efgh\n" if you have something in between, which can not be predefined. I.e. if you have

/// \brief Top level help.
static int helpMain(int argc, const char *argv[]) {
  errs() << "Usage: clang-rename {rename-at|rename-all} [OPTION]...\n\n"
         << "A tool to rename symbols in C/C++ code.\n\n"
         << "Subcommands:\n"
         << "  rename-at:  Perform rename off of a location in a file. (This is the default.)\n"
         << "  rename-all: Perform rename of all symbols matching one or more fully qualified names.\n";
  return 0;
}

Just make it a const string, isn't it better?

Done. I copied llvm-cov, but I have no problems changing it.

Cool.

3. tooling::RefactoringTool takes care of printing version IIUC, no need to do that in a custom way (we don't have custom version in clang-rename head at the moment, that was something Eugene.Zelenko sent recently).

Removed @; otherwise Eugene becomes subscribed :)

Indeed, removed.

4. clang-rename/RenamingAction.h -> USRList -> USRs or the other way around everywhere. So far single naming convention feels right to me (I personally prefer *s over *List). LLVM Coding Style does, too, from what I understand. Unless it's *Set, which is pretty much different thing.

I've changed NewNameList and PrevNameList.

USRList refers to a list of lists, i.e. doing one oldname->newname rename may
have to deal with multiple USRs, and when doing multiple oldname->newname
renames, you need to deal with a list of list of USRs. I used the List suffix
in this "list of list" case. I have no problem renaming
std::vector<std::vector<std::string>> USRList to USRs, but then we need a new
name for std::vector<std::string> USRs.

Aha, I see. Yes, for vector<vector>> it seems reasonable. Sorry, didn't that it's vector<vector>>.

5. Do we really need to dispatch these functions this way? with

enum RenameSubcommand {
  RenameAt, RenameAll
};

And many other strange things. Just pass bool isMultipleRename or isRenameAll to main routine instead of creating enum. We only have two such options here, right? I pray we won't have more.

I promise I don't plan to suggest more. :)

looking into your honest eyes

Changed the enum to a bool.

6. Move int main(int argc, const char **argv) upwards, so that it's above

dispatched routines. Otherwise when one wants to see what's going on, he sees
subroutine first without understanding where it comes from. Other way around
feels more intuitive to me.

Done.

As for help message, look at clang-tidy. Is there a need in helpMain?

[omtcyfz]

besides, let me push one thing; it's about passing a vector of USRs to the USRLocFinder instead of passing them 1 by 1; removes a need to write that FIXME of yours :)

[omtcyfz]

Hm, nevermind, I should check whether it doesn't break anything.

[omtcyfz]

Apparently it doesn't. Pushed to upstream.

[vmiklos]

Rebased on top of r277131 and resolved conflicts.

As for help message, look at clang-tidy. Is there a need in helpMain?

I think so; we have this chicken-and-egg problem (see earlier comments of this
review), that the options parser wants to know the option category, but we only
know the option category after parsing options due to subcommands. This is not
a problem for clang-tidy that doesn't have subcommands (as far as I see).

So one way is the own code for handling the subcommands (that's what I did
here, and that's what e.g. llvm-cov does), an other way would be to extend
tooling::CommonOptionsParser, so it doesn't want a category in the ctor. That
requirement is a problem for us, since we have two categories, so we can't give
the correct one without parsing the options.

So all in all, the best seems to me is to go with a simple helpMain().

besides, let me push one thing; it's about passing a vector of USRs to the USRLocFinder instead of passing them 1 by 1; removes a need to write that FIXME of yours :)

Great, I've removed it then.

[omtcyfz]

In D21814#500506, @vmiklos wrote:

Rebased on top of r277131 and resolved conflicts.

As for help message, look at clang-tidy. Is there a need in helpMain?

I think so; we have this chicken-and-egg problem (see earlier comments of this
review), that the options parser wants to know the option category, but we only
know the option category after parsing options due to subcommands. This is not
a problem for clang-tidy that doesn't have subcommands (as far as I see).

So one way is the own code for handling the subcommands (that's what I did
here, and that's what e.g. llvm-cov does), an other way would be to extend
tooling::CommonOptionsParser, so it doesn't want a category in the ctor. That
requirement is a problem for us, since we have two categories, so we can't give
the correct one without parsing the options.

So all in all, the best seems to me is to go with a simple helpMain().

Aha. I see. Well, it's not clean, but we can probably just deal with it later. Otherwise it gets too long and the patch is never going to be landed. Just write a FIXME then, I think I may look into that on the next week or somewhen.

besides, let me push one thing; it's about passing a vector of USRs to the USRLocFinder instead of passing them 1 by 1; removes a need to write that FIXME of yours :)

Great, I've removed it then.

One more paranoid comment:

class Cla1 { // CHECK: class Kla1

Most of the time I use Foo->Bar renaming in tests, just so that it could be uniform and everyone (including myself) would typically grep -FUbo "Foo" without looking at the source.

Other than that my concerns have been addressed.

Again, I'm not happy about this approach, but if that's a use-case we expect to have in the future and everyone is happy about it - let's do that.

[vmiklos]

Just write a FIXME then, I think I may look into that on the next week or somewhen.

Done.

Most of the time I use Foo->Bar renaming in tests

Done, I've renamed ClaN->KlaN to FooN->BarN.

[omtcyfz]

P.S. not sure whether we have to write clang-rename: for the -new-name option: must be specified out. We already launched clang-rename what else could've give us an error?

[vmiklos]

In D21814#500621, @omtcyfz wrote:

P.S. not sure whether we have to write clang-rename: for the -new-name option: must be specified out. We already launched clang-rename what else could've give us an error?

You mean how is that error produced? Previously there was no cl::Required on NewName (now: NewNames), and that's why a manual check was needed, the error message changed, as now the option parser does this checking for us.

[omtcyfz]

In D21814#500629, @vmiklos wrote:

In D21814#500621, @omtcyfz wrote:

P.S. not sure whether we have to write clang-rename: for the -new-name option: must be specified out. We already launched clang-rename what else could've give us an error?

You mean how is that error produced? Previously there was no cl::Required on NewName (now: NewNames), and that's why a manual check was needed, the error message changed, as now the option parser does this checking for us.

Ah, I see. So the message is generated, isn't it?

[vmiklos]

Yes, exactly, so not easy to customize I guess.

[omtcyfz]

In D21814#500735, @vmiklos wrote:

Yes, exactly, so not easy to customize I guess.

Aha, alright. Well, doesn't matter too much.

LGTM.

[vmiklos]

Great! Manuel, OK to land?

[vmiklos]

Rebased on top of r277339 and resolved conflicts.

[vmiklos]

Rebased on top of r277356 and resolved conflicts. (A busy day, it seems. :-) )

[klimek]

lg

[omtcyfz]

Make sure to rebase once more; documentation was updated in the last revision.

[vmiklos]

Yes, I did that -- but I got no conflicts there. ;-)

clang-tools-extra/trunk/clang-rename/RenamingAction.h

 namespace clang {
 class ASTConsumer;
 class CompilerInstance;
 
 namespace rename {
 
 class RenamingAction {
 public:
-  RenamingAction(const std::string &NewName, const std::string &PrevName,
-                 const std::vector<std::string> &USRs,
+  RenamingAction(const std::vector<std::string> &NewNames,
+                 const std::vector<std::string> &PrevNames,
+                 const std::vector<std::vector<std::string>> &USRList,
                  std::map<std::string, tooling::Replacements> &FileToReplaces,
                  bool PrintLocations = false)
-      : NewName(NewName), PrevName(PrevName), USRs(USRs),
+      : NewNames(NewNames), PrevNames(PrevNames), USRList(USRList),
         FileToReplaces(FileToReplaces), PrintLocations(PrintLocations) {}
 
   std::unique_ptr<ASTConsumer> newASTConsumer();
 
 private:
-  const std::string &NewName, &PrevName;
-  const std::vector<std::string> &USRs;
+  const std::vector<std::string> &NewNames, &PrevNames;
+  const std::vector<std::vector<std::string>> &USRList;
   std::map<std::string, tooling::Replacements> &FileToReplaces;
   bool PrintLocations;
 };
 
 }
 }
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_RENAME_RENAMING_ACTION_H_

clang-tools-extra/trunk/clang-rename/RenamingAction.cpp

 using namespace llvm;
 
 namespace clang {
 namespace rename {
 
 class RenamingASTConsumer : public ASTConsumer {
 public:
   RenamingASTConsumer(
-      const std::string &NewName, const std::string &PrevName,
-      const std::vector<std::string> &USRs,
+      const std::vector<std::string> &NewNames,
+      const std::vector<std::string> &PrevNames,
+      const std::vector<std::vector<std::string>> &USRList,
       std::map<std::string, tooling::Replacements> &FileToReplaces,
       bool PrintLocations)
-      : NewName(NewName), PrevName(PrevName), USRs(USRs),
+      : NewNames(NewNames), PrevNames(PrevNames), USRList(USRList),
         FileToReplaces(FileToReplaces), PrintLocations(PrintLocations) {}
 
   void HandleTranslationUnit(ASTContext &Context) override {
+    for (unsigned I = 0; I < NewNames.size(); ++I) {
+      HandleOneRename(Context, NewNames[I], PrevNames[I], USRList[I]);
+    }
+  }
+
+  void HandleOneRename(ASTContext &Context, const std::string &NewName,
+                       const std::string &PrevName,
+                       const std::vector<std::string> &USRs) {
     const auto &SourceMgr = Context.getSourceManager();
     std::vector<SourceLocation> RenamingCandidates;
     std::vector<SourceLocation> NewCandidates;
 
     NewCandidates =
         getLocationsOfUSRs(USRs, PrevName, Context.getTranslationUnitDecl());
     RenamingCandidates.insert(RenamingCandidates.end(), NewCandidates.begin(),
                               NewCandidates.end());
for (const auto &Loc : RenamingCandidates) {
       auto Err = FileToReplaces[Replace.getFilePath()].add(Replace);
       if (Err)
         llvm::errs() << "Renaming failed in " << Replace.getFilePath() << "! "
                      << llvm::toString(std::move(Err)) << "\n";
     }
   }
 
 private:
-  const std::string &NewName, &PrevName;
-  const std::vector<std::string> &USRs;
+  const std::vector<std::string> &NewNames, &PrevNames;
+  const std::vector<std::vector<std::string>> &USRList;
   std::map<std::string, tooling::Replacements> &FileToReplaces;
   bool PrintLocations;
 };
 
 std::unique_ptr<ASTConsumer> RenamingAction::newASTConsumer() {
-  return llvm::make_unique<RenamingASTConsumer>(NewName, PrevName, USRs,
+  return llvm::make_unique<RenamingASTConsumer>(NewNames, PrevNames, USRList,
                                                 FileToReplaces, PrintLocations);
 }
 
 } // namespace rename
 } // namespace clang

clang-tools-extra/trunk/clang-rename/tool/ClangRename.cpp

 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/YAMLTraits.h"
 #include <cstdlib>
 #include <string>
 #include <system_error>
 
 using namespace llvm;
 
-cl::OptionCategory ClangRenameCategory("Clang-rename options");
-
-static cl::opt<std::string>
-NewName(
-    "new-name",
-    cl::desc("The new name to change the symbol to."),
-    cl::cat(ClangRenameCategory));
-static cl::opt<unsigned>
-SymbolOffset(
-    "offset",
-    cl::desc("Locates the symbol by offset as opposed to <line>:<column>."),
-    cl::cat(ClangRenameCategory));
-static cl::opt<std::string>
-OldName(
-    "old-name",
-    cl::desc("The fully qualified name of the symbol, if -offset is not used."),
-    cl::cat(ClangRenameCategory));
-static cl::opt<bool>
-Inplace(
-    "i",
-    cl::desc("Overwrite edited <file>s."),
-    cl::cat(ClangRenameCategory));
-static cl::opt<bool>
-PrintName(
-    "pn",
-    cl::desc("Print the found symbol's name prior to renaming to stderr."),
-    cl::cat(ClangRenameCategory));
-static cl::opt<bool>
-PrintLocations(
-    "pl",
-    cl::desc("Print the locations affected by renaming to stderr."),
-    cl::cat(ClangRenameCategory));
-static cl::opt<std::string>
-ExportFixes(
-    "export-fixes",
-    cl::desc("YAML file to store suggested fixes in."),
-    cl::value_desc("filename"),
-    cl::cat(ClangRenameCategory));
-
 using namespace clang;
 
-const char RenameUsage[] = "A tool to rename symbols in C/C++ code.\n\
+cl::OptionCategory ClangRenameAtCategory("clang-rename rename-at options");
+cl::OptionCategory ClangRenameAllCategory("clang-rename rename-all options");
+
+const char RenameAtUsage[] = "A tool to rename symbols in C/C++ code.\n\
 clang-rename renames every occurrence of a symbol found at <offset> in\n\
 <source0>. If -i is specified, the edited files are overwritten to disk.\n\
 Otherwise, the results are written to stdout.\n";
 
+const char RenameAllUsage[] = "A tool to rename symbols in C/C++ code.\n\
+clang-rename renames every occurrence of a symbol named <old-name>.\n";
+
+static int renameAtMain(int argc, const char *argv[]);
+static int renameAllMain(int argc, const char *argv[]);
+static int helpMain(int argc, const char *argv[]);
+
 int main(int argc, const char **argv) {
-  tooling::CommonOptionsParser OP(argc, argv, ClangRenameCategory, RenameUsage);
+  if (argc > 1) {
+    using MainFunction = std::function<int(int, const char *[])>;
+    MainFunction Func = StringSwitch<MainFunction>(argv[1])
+                            .Case("rename-at", renameAtMain)
+                            .Case("rename-all", renameAllMain)
+                            .Cases("-help", "--help", helpMain)
+                            .Default(nullptr);
+
+    if (Func) {
+      std::string Invocation = std::string(argv[0]) + " " + argv[1];
+      argv[1] = Invocation.c_str();
+      return Func(argc - 1, argv + 1);
+    } else {
+      return renameAtMain(argc, argv);
+    }
+  }
 
-  // Check the arguments for correctness.
+  helpMain(argc, argv);
+  return 1;
+}
 
-  if (NewName.empty()) {
-    errs() << "ERROR: no new name provided.\n\n";
-    exit(1);
+int subcommandMain(bool isRenameAll, int argc, const char **argv) {
+  cl::OptionCategory *Category = nullptr;
+  const char *Usage = nullptr;
+  if (isRenameAll) {
+    Category = &ClangRenameAllCategory;
+    Usage = RenameAllUsage;
+  } else {
+    Category = &ClangRenameAtCategory;
+    Usage = RenameAtUsage;
   }
 
-  // Check if NewName is a valid identifier in C++17.
+  cl::list<std::string> NewNames(
+      "new-name", cl::desc("The new name to change the symbol to."),
+      (isRenameAll ? cl::OneOrMore : cl::Required), cl::cat(*Category));
+  cl::list<unsigned> SymbolOffsets(
+      "offset",
+      cl::desc("Locates the symbol by offset as opposed to <line>:<column>."),
+      (isRenameAll ? cl::ZeroOrMore : cl::Required), cl::cat(*Category));
+  cl::list<std::string> OldNames(
+      "old-name",
+      cl::desc(
+          "The fully qualified name of the symbol, if -offset is not used."),
+      (isRenameAll ? cl::ZeroOrMore : cl::Optional),
+      cl::cat(ClangRenameAllCategory));
+  cl::opt<bool> Inplace("i", cl::desc("Overwrite edited <file>s."),
+                        cl::cat(*Category));
+  cl::opt<bool> PrintName(
+      "pn",
+      cl::desc("Print the found symbol's name prior to renaming to stderr."),
+      cl::cat(ClangRenameAtCategory));
+  cl::opt<bool> PrintLocations(
+      "pl", cl::desc("Print the locations affected by renaming to stderr."),
+      cl::cat(ClangRenameAtCategory));
+  cl::opt<std::string> ExportFixes(
+      "export-fixes", cl::desc("YAML file to store suggested fixes in."),
+      cl::value_desc("filename"), cl::cat(*Category));
+
+  tooling::CommonOptionsParser OP(argc, argv, *Category, Usage);
+
+  // Check the arguments for correctness.
+
+  // Check if NewNames is a valid identifier in C++17.
+  for (const auto &NewName : NewNames) {
     LangOptions Options;
     Options.CPlusPlus = true;
     Options.CPlusPlus1z = true;
     IdentifierTable Table(Options);
     auto NewNameTokKind = Table.get(NewName).getTokenID();
     if (!tok::isAnyIdentifier(NewNameTokKind)) {
       errs() << "ERROR: new name is not a valid identifier in C++17.\n\n";
       exit(1);
     }
+  }
 
-  // Get the USRs.
+  if (!OldNames.empty() && OldNames.size() != NewNames.size()) {
+    errs() << "clang-rename: number of old names (" << OldNames.size()
+           << ") do not equal to number of new names (" << NewNames.size()
+           << ").\n\n";
+    cl::PrintHelpMessage();
+    exit(1);
+  }
+
+  if (!SymbolOffsets.empty() && SymbolOffsets.size() != NewNames.size()) {
+    errs() << "clang-rename: number of symbol offsets (" << SymbolOffsets.size()
+           << ") do not equal to number of new names (" << NewNames.size()
+           << ").\n\n";
+    cl::PrintHelpMessage();
+    exit(1);
+  }
+
+  std::vector<std::vector<std::string>> USRList;
+  std::vector<std::string> PrevNames;
   auto Files = OP.getSourcePathList();
   tooling::RefactoringTool Tool(OP.getCompilations(), Files);
+  unsigned Count = OldNames.size() ? OldNames.size() : SymbolOffsets.size();
+  for (unsigned I = 0; I < Count; ++I) {
+    unsigned SymbolOffset = SymbolOffsets.empty() ? 0 : SymbolOffsets[I];
+    const std::string &OldName = OldNames.empty() ? std::string() : OldNames[I];
+
+    // Get the USRs.
     rename::USRFindingAction USRAction(SymbolOffset, OldName);
 
     // Find the USRs.
     Tool.run(tooling::newFrontendActionFactory(&USRAction).get());
     const auto &USRs = USRAction.getUSRs();
+    USRList.push_back(USRs);
     const auto &PrevName = USRAction.getUSRSpelling();
+    PrevNames.push_back(PrevName);
 
     if (PrevName.empty()) {
       // An error should have already been printed.
       exit(1);
     }
 
     if (PrintName) {
       errs() << "clang-rename: found name: " << PrevName << '\n';
     }
+  }
 
   // Perform the renaming.
-  rename::RenamingAction RenameAction(NewName, PrevName, USRs,
+  rename::RenamingAction RenameAction(NewNames, PrevNames, USRList,
                                       Tool.getReplacements(), PrintLocations);
   auto Factory = tooling::newFrontendActionFactory(&RenameAction);
   int ExitCode;
 
   if (Inplace) {
     ExitCode = Tool.runAndSave(Factory.get());
   } else {
     ExitCode = Tool.run(Factory.get());
if (!ExportFixes.empty()) {
       OS.close();
       exit(0);
     }
 
     // Write every file to stdout. Right now we just barf the files without any
     // indication of which files start where, other than that we print the files
     // in the same order we see them.
     LangOptions DefaultLangOptions;
-    IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts =
-        new DiagnosticOptions();
+    IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
     TextDiagnosticPrinter DiagnosticPrinter(errs(), &*DiagOpts);
     DiagnosticsEngine Diagnostics(
-        IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()),
-        &*DiagOpts, &DiagnosticPrinter, false);
+        IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), &*DiagOpts,
+        &DiagnosticPrinter, false);
     auto &FileMgr = Tool.getFiles();
     SourceManager Sources(Diagnostics, FileMgr);
     Rewriter Rewrite(Sources, DefaultLangOptions);
 
     Tool.applyAllReplacements(Rewrite);
     for (const auto &File : Files) {
       const auto *Entry = FileMgr.getFile(File);
       auto ID = Sources.translateFile(Entry);
       Rewrite.getEditBuffer(ID).write(outs());
     }
   }
 
   exit(ExitCode);
 }
+
+/// \brief Top level help.
+/// FIXME It would be better if this could be auto-generated.
+static int helpMain(int argc, const char *argv[]) {
+  errs() << "Usage: clang-rename {rename-at|rename-all} [OPTION]...\n\n"
+            "A tool to rename symbols in C/C++ code.\n\n"
+            "Subcommands:\n"
+            "  rename-at:  Perform rename off of a location in a file. (This "
+            "is the default.)\n"
+            "  rename-all: Perform rename of all symbols matching one or more "
+            "fully qualified names.\n";
+  return 0;
+}
+
+static int renameAtMain(int argc, const char *argv[]) {
+  return subcommandMain(false, argc, argv);
+}
+
+static int renameAllMain(int argc, const char *argv[]) {
+  return subcommandMain(true, argc, argv);
+}

clang-tools-extra/trunk/docs/clang-rename.rst

 
 To get an offset of a symbol in a file run
 
 .. code-block:: console
 
   $ grep -FUbo 'foo' file.cpp
 
 
+You can also identify one or more symbols to be renamed by giving the fully qualified
+name:
+
+.. code-block:: console
+
+  $ clang-rename rename-all -old-name=foo -new-name=bar test.cpp
+
+
 The tool currently supports renaming actions inside a single Translation Unit
 only. It is planned to extend the tool's functionality to support multi-TU
 renaming actions in the future.
 
 :program:`clang-rename` also aims to be easily integrated into popular text
 editors, such as Vim and Emacs, and improve the workflow of users.
 
 Although a command line interface exists, it is highly recommended to use the
 text editor interface instead for better experience.
 
 .. code-block:: console
 
   $ clang-rename -help
+  Usage: clang-rename {rename-at|rename-all} [OPTION]...
+
+  A tool to rename symbols in C/C++ code.
+
+  Subcommands:
+    rename-at:  Perform rename off of a location in a file. (This is the default.)
+    rename-all: Perform rename of all symbols matching one or more fully qualified names.
+
+
+.. code-block:: console
+
+  $ clang-rename rename-at -help
   OVERVIEW: A tool to rename symbols in C/C++ code.
   clang-rename renames every occurrence of a symbol found at <offset> in
   <source0>. If -i is specified, the edited files are overwritten to disk.
   Otherwise, the results are written to stdout.
   
-  USAGE: clang-rename [subcommand] [options] <source0> [... <sourceN>]
+  USAGE: clang-rename rename-at [subcommand] [options] <source0> [... <sourceN>]
   
   OPTIONS:
   
-  Clang-rename options:
+  Generic Options:
+  
+    -help                      - Display available options (-help-hidden for more)
+    -help-list                 - Display list of available options (-help-list-hidden for more)
+    -version                   - Display the version of this program
+
+  clang-rename rename-at options:
 
     -export-fixes=<filename>   - YAML file to store suggested fixes in.
     -extra-arg=<string>        - Additional argument to append to the compiler command line
     -extra-arg-before=<string> - Additional argument to prepend to the compiler command line
     -i                         - Overwrite edited <file>s.
     -new-name=<string>         - The new name to change the symbol to.
     -offset=<uint>             - Locates the symbol by offset as opposed to <line>:<column>.
-    -old-name=<string>         - The fully qualified name of the symbol, if -offset is not used.
     -p=<string>                - Build path
     -pl                        - Print the locations affected by renaming to stderr.
     -pn                        - Print the found symbol's name prior to renaming to stderr.
 
+
+.. code-block:: console
+
+  $ clang-rename rename-all -help
+  OVERVIEW: A tool to rename symbols in C/C++ code.
+  clang-rename renames every occurrence of a symbol named <old-name>.
+
+  USAGE: clang-rename rename-all [subcommand] [options] <source0> [... <sourceN>]
+
+  OPTIONS:
+
   Generic Options:
 
     -help                      - Display available options (-help-hidden for more)
     -help-list                 - Display list of available options (-help-list-hidden for more)
     -version                   - Display the version of this program
 
+  clang-rename rename-all options:
+
+    -export-fixes=<filename>   - YAML file to store suggested fixes in.
+    -extra-arg=<string>        - Additional argument to append to the compiler command line
+    -extra-arg-before=<string> - Additional argument to prepend to the compiler command line
+    -i                         - Overwrite edited <file>s.
+    -new-name=<string>         - The new name to change the symbol to.
+    -offset=<uint>             - Locates the symbol by offset as opposed to <line>:<column>.
+    -old-name=<string>         - The fully qualified name of the symbol, if -offset is not used.
+    -p=<string>                - Build path
+
 
 Vim Integration
 ===============
 
 You can call :program:`clang-rename` directly from Vim! To set up
 :program:`clang-rename` integration for Vim see
 `clang-rename/tool/clang-rename.py
 <http://reviews.llvm.org/diffusion/L/browse/clang-tools-extra/trunk/clang-rename/tool/clang-rename.py>`_.

clang-tools-extra/trunk/test/clang-rename/ClassFindByName.cpp

 // RUN: cat %s > %t.cpp
-// RUN: clang-rename -old-name=Foo -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %t.cpp -i --
 // RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
 
 class Foo {         // CHECK: class Bar
 };
 
 int main() {
   Foo *Pointer = 0; // CHECK: Bar *Pointer = 0;
   return 0;
 }

clang-tools-extra/trunk/test/clang-rename/ClassTestMulti.cpp

+// RUN: cat %s > %t.cpp
+// RUN: clang-rename rename-all -offset=174 -new-name=Bar1 -offset=212 -new-name=Bar2 %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+class Foo1 { // CHECK: class Bar1
+};
+
+class Foo2 { // CHECK: class Bar2
+};

clang-tools-extra/trunk/test/clang-rename/ClassTestMultiByName.cpp

+// RUN: cat %s > %t.cpp
+// RUN: clang-rename rename-all -old-name=Foo1 -new-name=Bar1 -old-name=Foo2 -new-name=Bar2 %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+class Foo1 { // CHECK: class Bar1
+};
+
+class Foo2 { // CHECK: class Bar2
+};

clang-tools-extra/trunk/test/clang-rename/FunctionWithClassFindByName.cpp

 // RUN: cat %s > %t.cpp
-// RUN: clang-rename -old-name=Foo -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %t.cpp -i --
 // RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
 
 void foo() {
 }
 
 class Foo {         // CHECK: class Bar
 };
 
 int main() {
   Foo *Pointer = 0; // CHECK: Bar *Pointer = 0;
   return 0;
 }
 

clang-tools-extra/trunk/test/clang-rename/NoNewName.cpp

 // Check for an error while -new-name argument has not been passed to
 // clang-rename.
 // RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: ERROR: no new name provided.
+// CHECK: clang-rename: for the -new-name option: must be specified