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

[Kirill Bobyrev via cfe-commits]

omtcyfz added a comment.

In https://reviews.llvm.org/D21814#486269, @vmiklos wrote:

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


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.


https://reviews.llvm.org/D21814