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
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&id=66448#toc
Repository:
rL LLVM
https:
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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c
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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com
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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commit
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
test/c
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
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
test/c
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
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 list
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
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` wh
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
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
___
cfe-commit
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
___
vmiklos updated this revision to Diff 66110.
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
test/c
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
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, b
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
test/c
omtcyfz added a comment.
Apparently it doesn't. Pushed to upstream.
https://reviews.llvm.org/D21814
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
omtcyfz added a comment.
Hm, nevermind, I should check whether it doesn't break anything.
https://reviews.llvm.org/D21814
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
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 maili
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 resu
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 b
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
test/c
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
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
http
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
http://lists.llvm.org/cgi-bin/mailman/listinfo
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
test/c
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
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
test/c
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 necess
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
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
test/clan
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
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
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
test/clan
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
test/clan
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
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.
https://reviews.l
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,
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
http://
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
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
test/clan
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], Prev
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 whet
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 down
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
test/clan
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
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
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 lo
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
test/clan
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 it'
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
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
http:
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
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` i
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
test/clang-rename/ClassSimpleRena
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 add
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 a
60 matches
Mail list logo