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