[PATCH] D21814: clang-rename: split existing options into two new subcommands

[Miklos Vajna via cfe-commits]

vmiklos added a comment.

In https://reviews.llvm.org/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.


https://reviews.llvm.org/D21814