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