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

vmiklos added a comment. Yes, I did that -- but I got no conflicts there. ;-) Repository: rL LLVM https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL277438: clang-rename: split existing options into two new subcommands (authored by vmiklos). Changed prior to commit: https://reviews.llvm.org/D21814?vs=66362=66448#toc Repository: rL LLVM

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

omtcyfz added a comment. //Make sure to rebase once more; documentation was updated in the last revision.// https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

vmiklos added a comment. Rebased on top of r277356 and resolved conflicts. (A busy day, it seems. :-) ) https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

vmiklos updated this revision to Diff 66362. https://reviews.llvm.org/D21814 Files: clang-rename/RenamingAction.cpp clang-rename/RenamingAction.h clang-rename/tool/ClangRename.cpp docs/clang-rename.rst test/clang-rename/ClassFindByName.cpp test/clang-rename/ClassTestMulti.cpp

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

vmiklos added a comment. Rebased on top of r277339 and resolved conflicts. https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

vmiklos updated this revision to Diff 66304. https://reviews.llvm.org/D21814 Files: clang-rename/RenamingAction.cpp clang-rename/RenamingAction.h clang-rename/tool/ClangRename.cpp docs/clang-rename.rst test/clang-rename/ClassFindByName.cpp test/clang-rename/ClassTestMulti.cpp

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

vmiklos added a comment. Great! Manuel, OK to land? https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

omtcyfz added a comment. In https://reviews.llvm.org/D21814#500735, @vmiklos wrote: > Yes, exactly, so not easy to customize I guess. Aha, alright. Well, doesn't matter too much. LGTM. https://reviews.llvm.org/D21814 ___ cfe-commits mailing

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

vmiklos added a comment. Yes, exactly, so not easy to customize I guess. https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

omtcyfz added a comment. In https://reviews.llvm.org/D21814#500629, @vmiklos wrote: > In https://reviews.llvm.org/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`

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

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

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

omtcyfz added a comment. 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? https://reviews.llvm.org/D21814 ___

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

vmiklos added a comment. > 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. https://reviews.llvm.org/D21814

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

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

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

vmiklos added a comment. 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,

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

vmiklos updated this revision to Diff 66105. https://reviews.llvm.org/D21814 Files: clang-rename/RenamingAction.cpp clang-rename/RenamingAction.h clang-rename/tool/ClangRename.cpp docs/clang-rename.rst test/clang-rename/ClassFindByName.cpp test/clang-rename/ClassTestMulti.cpp

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

omtcyfz added a comment. 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 :) https://reviews.llvm.org/D21814 ___ cfe-commits

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

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

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

vmiklos added a subscriber: Eugene.Zelenko. vmiklos added a comment. > 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

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

vmiklos updated this revision to Diff 66099. https://reviews.llvm.org/D21814 Files: clang-rename/RenamingAction.cpp clang-rename/RenamingAction.h clang-rename/tool/ClangRename.cpp docs/clang-rename.rst test/clang-rename/ClassFindByName.cpp test/clang-rename/ClassTestMulti.cpp

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

omtcyfz added a subscriber: Eugene.Zelenko. omtcyfz added a comment. 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

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

vmiklos added a comment. 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. https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

vmiklos added a comment. Rebased on top of r276949 and resolved a failing test (FunctionWithClassFindByName.cpp). https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

vmiklos updated this revision to Diff 65876. https://reviews.llvm.org/D21814 Files: clang-rename/RenamingAction.cpp clang-rename/RenamingAction.h clang-rename/tool/ClangRename.cpp docs/clang-rename.rst test/clang-rename/ClassFindByName.cpp test/clang-rename/ClassTestMulti.cpp

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

vmiklos marked 2 inline comments as done. vmiklos added a comment. > 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

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

vmiklos updated this revision to Diff 65684. https://reviews.llvm.org/D21814 Files: clang-rename/RenamingAction.cpp clang-rename/RenamingAction.h clang-rename/tool/ClangRename.cpp docs/clang-rename.rst test/clang-rename/ClassFindByName.cpp test/clang-rename/ClassTestMulti.cpp

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

omtcyfz added a comment. Apart from my high level concerns, which of course I still have... Comment at: clang-rename/tool/clang-rename.py:43 @@ -42,2 +42,3 @@ command = [binary, + "rename-at", filename, `rename-at` isn't

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

vmiklos added a comment. Rebased on top of r276836 and resolved conflicts. https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

vmiklos updated this revision to Diff 65672. https://reviews.llvm.org/D21814 Files: clang-rename/RenamingAction.cpp clang-rename/RenamingAction.h clang-rename/tool/ClangRename.cpp clang-rename/tool/clang-rename.py docs/clang-rename.rst test/clang-rename/ClassFindByName.cpp

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

klimek added a comment. Kirill, are your specific concerns addressed? https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

vmiklos added a comment. Rebased on top of r276684 and resolved conflicts. https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

vmiklos updated this revision to Diff 65417. https://reviews.llvm.org/D21814 Files: clang-rename/RenamingAction.cpp clang-rename/RenamingAction.h clang-rename/tool/ClangRename.cpp clang-rename/tool/clang-rename.py docs/clang-rename.rst test/clang-rename/ClassFindByName.cpp

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

vmiklos updated this revision to Diff 65042. https://reviews.llvm.org/D21814 Files: clang-rename/RenamingAction.cpp clang-rename/RenamingAction.h clang-rename/tool/ClangRename.cpp clang-rename/tool/clang-rename.py docs/clang-rename.rst test/clang-rename/ClassFindByName.cpp

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

vmiklos added a comment. Done, that also allows not modifying most existing tests. https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

omtcyfz added a comment. > 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.

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

vmiklos added a comment. In https://reviews.llvm.org/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,

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

omtcyfz added a comment. I'd be actually happy if instead of having `-rename-at` option we'd have this behavior by default unless `-rename-all` is used. https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

vmiklos marked an inline comment as done. vmiklos added a comment. https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

vmiklos updated this revision to Diff 65031. https://reviews.llvm.org/D21814 Files: clang-rename/RenamingAction.cpp clang-rename/RenamingAction.h clang-rename/tool/ClangRename.cpp clang-rename/tool/clang-rename.py docs/clang-rename.rst test/clang-rename/ClassFindByName.cpp

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

klimek added a comment. Kirill, unless you have *specific* issues with this patch, I think it's good to land. Comment at: clang-rename/RenamingAction.cpp:48 @@ +47,3 @@ +for (unsigned I = 0; I < NewNameList.size(); ++I) { + HandleOneRename(Context, NewNameList[I],

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

vmiklos added inline comments. Comment at: clang-rename/RenamingAction.cpp:48 @@ +47,3 @@ +for (unsigned I = 0; I < NewNameList.size(); ++I) { + HandleOneRename(Context, NewNameList[I], PrevNameList[I], USRList[I]); +} klimek wrote: > Question is

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

klimek added inline comments. Comment at: clang-rename/RenamingAction.cpp:48 @@ +47,3 @@ +for (unsigned I = 0; I < NewNameList.size(); ++I) { + HandleOneRename(Context, NewNameList[I], PrevNameList[I], USRList[I]); +} Question is whether if we go

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

vmiklos updated this revision to Diff 64941. https://reviews.llvm.org/D21814 Files: clang-rename/RenamingAction.cpp clang-rename/RenamingAction.h clang-rename/tool/ClangRename.cpp clang-rename/tool/clang-rename.py docs/clang-rename.rst test/clang-rename/ClassFindByName.cpp

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

vmiklos added a comment. Rebased on top of r276282 and resolved conflicts. https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

vmiklos marked an inline comment as done. vmiklos added a comment. https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

vmiklos added a comment. > 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

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

vmiklos updated this revision to Diff 64859. https://reviews.llvm.org/D21814 Files: clang-rename/RenamingAction.cpp clang-rename/RenamingAction.h clang-rename/tool/ClangRename.cpp clang-rename/tool/clang-rename.py docs/clang-rename.rst test/clang-rename/ClassFindByName.cpp

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

omtcyfz added a comment. 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

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

klimek added a comment. Kirill, you happy with this approach? https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

vmiklos added a comment. 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. https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

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

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

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`

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

vmiklos updated this revision to Diff 64231. https://reviews.llvm.org/D21814 Files: clang-rename/tool/ClangRename.cpp clang-rename/tool/clang-rename.py docs/clang-rename.rst test/clang-rename/ClassFindByName.cpp test/clang-rename/ClassReplacements.cpp

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

omtcyfz added a subscriber: omtcyfz. omtcyfz added a comment. - 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

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

vmiklos added a comment. 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