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
https://reviews.llvm.org/D21814
Files:
clang-tools-extra/trunk/clang-rename/RenamingAction.cpp
clang-tools-extra/trunk/clang-rename/RenamingAction.h
clang-tools-extra/trunk/clang-rename/tool/ClangRename.cpp
clang-tools-extra/trunk/docs/clang-rename.rst
clang-tools-extra/trunk/test/clang-rename/ClassFindByName.cpp
clang-tools-extra/trunk/test/clang-rename/ClassTestMulti.cpp
clang-tools-extra/trunk/test/clang-rename/ClassTestMultiByName.cpp
clang-tools-extra/trunk/test/clang-rename/FunctionWithClassFindByName.cpp
clang-tools-extra/trunk/test/clang-rename/NoNewName.cpp
Index: clang-tools-extra/trunk/test/clang-rename/NoNewName.cpp
===
--- clang-tools-extra/trunk/test/clang-rename/NoNewName.cpp
+++ clang-tools-extra/trunk/test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: ERROR: no new name provided.
+// CHECK: clang-rename: for the -new-name option: must be specified
Index: clang-tools-extra/trunk/test/clang-rename/ClassTestMultiByName.cpp
===
--- clang-tools-extra/trunk/test/clang-rename/ClassTestMultiByName.cpp
+++ clang-tools-extra/trunk/test/clang-rename/ClassTestMultiByName.cpp
@@ -0,0 +1,8 @@
+// RUN: cat %s > %t.cpp
+// RUN: clang-rename rename-all -old-name=Foo1 -new-name=Bar1 -old-name=Foo2 -new-name=Bar2 %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+class Foo1 { // CHECK: class Bar1
+};
+
+class Foo2 { // CHECK: class Bar2
+};
Index: clang-tools-extra/trunk/test/clang-rename/ClassFindByName.cpp
===
--- clang-tools-extra/trunk/test/clang-rename/ClassFindByName.cpp
+++ clang-tools-extra/trunk/test/clang-rename/ClassFindByName.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -old-name=Foo -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Foo { // CHECK: class Bar
Index: clang-tools-extra/trunk/test/clang-rename/ClassTestMulti.cpp
===
--- clang-tools-extra/trunk/test/clang-rename/ClassTestMulti.cpp
+++ clang-tools-extra/trunk/test/clang-rename/ClassTestMulti.cpp
@@ -0,0 +1,8 @@
+// RUN: cat %s > %t.cpp
+// RUN: clang-rename rename-all -offset=174 -new-name=Bar1 -offset=212 -new-name=Bar2 %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+class Foo1 { // CHECK: class Bar1
+};
+
+class Foo2 { // CHECK: class Bar2
+};
Index: clang-tools-extra/trunk/test/clang-rename/FunctionWithClassFindByName.cpp
===
--- clang-tools-extra/trunk/test/clang-rename/FunctionWithClassFindByName.cpp
+++ clang-tools-extra/trunk/test/clang-rename/FunctionWithClassFindByName.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -old-name=Foo -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
void foo() {
Index: clang-tools-extra/trunk/clang-rename/RenamingAction.cpp
===
--- clang-tools-extra/trunk/clang-rename/RenamingAction.cpp
+++ clang-tools-extra/trunk/clang-rename/RenamingAction.cpp
@@ -35,14 +35,23 @@
class RenamingASTConsumer : public ASTConsumer {
public:
RenamingASTConsumer(
- const std::string , const std::string ,
- const std::vector ,
+ const std::vector ,
+ const std::vector ,
+ const std::vector ,
std::map ,
bool PrintLocations)
- : NewName(NewName), PrevName(PrevName), USRs(USRs),
+ : NewNames(NewNames), PrevNames(PrevNames), USRList(USRList),
FileToReplaces(FileToReplaces), PrintLocations(PrintLocations) {}
void HandleTranslationUnit(ASTContext ) override {
+for (unsigned I = 0; I < NewNames.size(); ++I) {
+ HandleOneRename(Context, NewNames[I], PrevNames[I], USRList[I]);
+}
+ }
+
+ void HandleOneRename(ASTContext , const std::string ,
+ const std::string ,
+ const std::vector ) {
const auto = Context.getSourceManager();
std::vector RenamingCandidates;
std::vector NewCandidates;
@@ -70,14 +79,14 @@
}
private:
- const std::string ,
- const std::vector
+ const std::vector ,
+ const std::vector
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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
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 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 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-commits
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
test/clang-rename/ClassTestMultiByName.cpp
test/clang-rename/FunctionWithClassFindByName.cpp
test/clang-rename/NoNewName.cpp
Index: test/clang-rename/NoNewName.cpp
===
--- test/clang-rename/NoNewName.cpp
+++ test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: ERROR: no new name provided.
+// CHECK: clang-rename: for the -new-name option: must be specified
Index: test/clang-rename/FunctionWithClassFindByName.cpp
===
--- test/clang-rename/FunctionWithClassFindByName.cpp
+++ test/clang-rename/FunctionWithClassFindByName.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -old-name=Foo -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
void foo() {
Index: test/clang-rename/ClassTestMultiByName.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMultiByName.cpp
@@ -0,0 +1,8 @@
+// RUN: cat %s > %t.cpp
+// RUN: clang-rename rename-all -old-name=Foo1 -new-name=Bar1 -old-name=Foo2 -new-name=Bar2 %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+class Foo1 { // CHECK: class Bar1
+};
+
+class Foo2 { // CHECK: class Bar2
+};
Index: test/clang-rename/ClassTestMulti.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMulti.cpp
@@ -0,0 +1,8 @@
+// RUN: cat %s > %t.cpp
+// RUN: clang-rename rename-all -offset=174 -new-name=Bar1 -offset=212 -new-name=Bar2 %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+class Foo1 { // CHECK: class Bar1
+};
+
+class Foo2 { // CHECK: class Bar2
+};
Index: test/clang-rename/ClassFindByName.cpp
===
--- test/clang-rename/ClassFindByName.cpp
+++ test/clang-rename/ClassFindByName.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -old-name=Foo -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Foo { // CHECK: class Bar
Index: docs/clang-rename.rst
===
--- docs/clang-rename.rst
+++ docs/clang-rename.rst
@@ -42,6 +42,14 @@
$ grep -FUbo 'foo' file.cpp
+You can also identify one or more symbols to be renamed by giving the fully qualified
+name:
+
+.. code-block:: console
+
+ $ clang-rename rename-all -old-name=foo -new-name=bar test.cpp
+
+
The tool currently supports renaming actions inside a single Translation Unit
only. It is planned to extend the tool's functionality to support multi-TU
renaming actions in the future.
@@ -55,34 +63,73 @@
.. code-block:: console
$ clang-rename -help
+ Usage: clang-rename {rename-at|rename-all} [OPTION]...
+
+ A tool to rename symbols in C/C++ code.
+
+ Subcommands:
+rename-at: Perform rename off of a location in a file. (This is the default.)
+rename-all: Perform rename of all symbols matching one or more fully qualified names.
+
+
+.. code-block:: console
+
+ $ clang-rename rename-at -help
OVERVIEW: A tool to rename symbols in C/C++ code.
clang-rename renames every occurrence of a symbol found at in
. If -i is specified, the edited files are overwritten to disk.
Otherwise, the results are written to stdout.
-
- USAGE: clang-rename [subcommand] [options] [... ]
-
+
+ USAGE: clang-rename rename-at [subcommand] [options] [... ]
+
OPTIONS:
+
+ Generic Options:
+
+-help - Display available options (-help-hidden for more)
+-help-list - Display list of available options (-help-list-hidden for more)
+-version - Display the version of this program
- Clang-rename options:
+ clang-rename rename-at options:
-export-fixes= - YAML file to store suggested fixes in.
-extra-arg=- Additional argument to append to the compiler command line
-extra-arg-before= - Additional argument to prepend to the compiler command line
-i - Overwrite edited s.
-new-name= - The new name to change the symbol to.
-offset= - Locates the symbol by offset as opposed to :.
--old-name= - The fully qualified name of the symbol, if -offset is not used.
-p=- Build path
-pl
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
test/clang-rename/ClassTestMultiByName.cpp
test/clang-rename/FunctionWithClassFindByName.cpp
test/clang-rename/NoNewName.cpp
Index: test/clang-rename/NoNewName.cpp
===
--- test/clang-rename/NoNewName.cpp
+++ test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: ERROR: no new name provided.
+// CHECK: clang-rename: for the -new-name option: must be specified
Index: test/clang-rename/FunctionWithClassFindByName.cpp
===
--- test/clang-rename/FunctionWithClassFindByName.cpp
+++ test/clang-rename/FunctionWithClassFindByName.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-rename -old-name=Foo -new-name=Bar %s -- | FileCheck %s
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %s -- | FileCheck %s
void foo() {
}
Index: test/clang-rename/ClassTestMultiByName.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMultiByName.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-rename rename-all -old-name=Foo1 -new-name=Bar1 -old-name=Foo2 -new-name=Bar2 %s -- | FileCheck %s
+class Foo1 { // CHECK: class Bar1
+};
+
+class Foo2 { // CHECK: class Bar2
+};
Index: test/clang-rename/ClassTestMulti.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMulti.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-rename rename-all -offset=113 -new-name=Bar1 -offset=151 -new-name=Bar2 %s -- | FileCheck %s
+class Foo1 { // CHECK: class Bar1
+};
+
+class Foo2 { // CHECK: class Bar2
+};
Index: test/clang-rename/ClassFindByName.cpp
===
--- test/clang-rename/ClassFindByName.cpp
+++ test/clang-rename/ClassFindByName.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-rename -old-name=Foo -new-name=Bar %s -- | FileCheck %s
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %s -- | FileCheck %s
class Foo { // CHECK: class Bar
};
Index: docs/clang-rename.rst
===
--- docs/clang-rename.rst
+++ docs/clang-rename.rst
@@ -42,6 +42,14 @@
$ grep -FUbo 'foo' file.cpp
+You can also identify one or more symbols to be renamed by giving the fully qualified
+name:
+
+.. code-block:: console
+
+ $ clang-rename rename-all -old-name=foo -new-name=bar test.cpp
+
+
The tool currently supports renaming actions inside a single Translation Unit
only. It is planned to extend the tool's functionality to support multi-TU
renaming actions in the future.
@@ -55,34 +63,73 @@
.. code-block:: console
$ clang-rename -help
+ Usage: clang-rename {rename-at|rename-all} [OPTION]...
+
+ A tool to rename symbols in C/C++ code.
+
+ Subcommands:
+rename-at: Perform rename off of a location in a file. (This is the default.)
+rename-all: Perform rename of all symbols matching one or more fully qualified names.
+
+
+.. code-block:: console
+
+ $ clang-rename rename-at -help
OVERVIEW: A tool to rename symbols in C/C++ code.
clang-rename renames every occurrence of a symbol found at in
. If -i is specified, the edited files are overwritten to disk.
Otherwise, the results are written to stdout.
-
- USAGE: clang-rename [subcommand] [options] [... ]
-
+
+ USAGE: clang-rename rename-at [subcommand] [options] [... ]
+
OPTIONS:
+
+ Generic Options:
+
+-help - Display available options (-help-hidden for more)
+-help-list - Display list of available options (-help-list-hidden for more)
+-version - Display the version of this program
- Clang-rename options:
+ clang-rename rename-at options:
-export-fixes= - YAML file to store suggested fixes in.
-extra-arg=- Additional argument to append to the compiler command line
-extra-arg-before= - Additional argument to prepend to the compiler command line
-i - Overwrite edited s.
-new-name= - The new name to change the symbol to.
-offset= - Locates the symbol by offset as opposed to :.
--old-name= - The fully qualified name of the symbol, if -offset is not used.
-p=- Build path
-pl- Print the locations affected by renaming to stderr.
-pn- Print the found symbol's name prior to renaming to stderr.
+
+.. code-block:: console
+
+ $
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 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. 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` what > > else could've give us an error? > > > You mean how is that error produced? Previously there was no `cl::Required` > on NewName (now: NewNames), and that's why a manual check was needed, the > error message changed, as now the option parser does this checking for us. Ah, I see. So the message is generated, isn't it? 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. 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 produced? Previously there was no `cl::Required` on NewName (now: NewNames), and that's why a manual check was needed, the error message changed, as now the option parser does this checking for us. 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. 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-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. > 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 ___ 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#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
> review), that the options parser wants to know the option category, but we
> only
> know the option category after parsing options due to subcommands. This is
> not
> a problem for clang-tidy that doesn't have subcommands (as far as I see).
>
> So one way is the own code for handling the subcommands (that's what I did
> here, and that's what e.g. llvm-cov does), an other way would be to extend
> `tooling::CommonOptionsParser`, so it doesn't want a category in the ctor.
> That
> requirement is a problem for us, since we have two categories, so we can't
> give
> the correct one without parsing the options.
>
> So all in all, the best seems to me is to go with a simple helpMain().
Aha. I see. Well, it's not clean, but we can probably just deal with it later.
Otherwise it gets too long and the patch is never going to be landed. Just
write a `FIXME` then, I think I may look into that on the next week or somewhen.
>
>
> > 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 :)
>
>
> Great, I've removed it then.
One more paranoid comment:
`class Cla1 { // CHECK: class Kla1`
Most of the time I use `Foo`->`Bar` renaming in tests, just so that it could be
uniform and everyone (including myself) would typically `grep -FUbo "Foo"`
without looking at the source.
Other than that my concerns have been addressed.
Again, I'm not happy about this approach, but if that's a use-case we expect to
have in the future and everyone is happy about it - let's do 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
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, but we only know the option category after parsing options due to subcommands. This is not a problem for clang-tidy that doesn't have subcommands (as far as I see). So one way is the own code for handling the subcommands (that's what I did here, and that's what e.g. llvm-cov does), an other way would be to extend `tooling::CommonOptionsParser`, so it doesn't want a category in the ctor. That requirement is a problem for us, since we have two categories, so we can't give the correct one without parsing the options. So all in all, the best seems to me is to go with a simple helpMain(). > 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 :) Great, I've removed it then. 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 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/clang-rename/ClassTestMultiByName.cpp
test/clang-rename/FunctionWithClassFindByName.cpp
test/clang-rename/NoNewName.cpp
Index: test/clang-rename/NoNewName.cpp
===
--- test/clang-rename/NoNewName.cpp
+++ test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: ERROR: no new name provided.
+// CHECK: clang-rename: for the -new-name option: must be specified
Index: test/clang-rename/FunctionWithClassFindByName.cpp
===
--- test/clang-rename/FunctionWithClassFindByName.cpp
+++ test/clang-rename/FunctionWithClassFindByName.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-rename -old-name=Foo -new-name=Bar %s -- | FileCheck %s
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %s -- | FileCheck %s
void foo() {
}
Index: test/clang-rename/ClassTestMultiByName.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMultiByName.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-rename rename-all -old-name=Cla1 -new-name=Kla1 -old-name=Cla2 -new-name=Kla2 %s -- | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassTestMulti.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMulti.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-rename rename-all -offset=113 -new-name=Kla1 -offset=151 -new-name=Kla2 %s -- | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassFindByName.cpp
===
--- test/clang-rename/ClassFindByName.cpp
+++ test/clang-rename/ClassFindByName.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-rename -old-name=Foo -new-name=Bar %s -- | FileCheck %s
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %s -- | FileCheck %s
class Foo { // CHECK: class Bar
};
Index: docs/clang-rename.rst
===
--- docs/clang-rename.rst
+++ docs/clang-rename.rst
@@ -42,6 +42,14 @@
$ grep -FUbo 'foo' file.cpp
+You can also identify one or more symbols to be renamed by giving the fully qualified
+name:
+
+.. code-block:: console
+
+ $ clang-rename rename-all -old-name=foo -new-name=bar test.cpp
+
+
The tool currently supports renaming actions inside a single Translation Unit
only. It is planned to extend the tool's functionality to support multi-TU
renaming actions in the future.
@@ -55,34 +63,73 @@
.. code-block:: console
$ clang-rename -help
+ Usage: clang-rename {rename-at|rename-all} [OPTION]...
+
+ A tool to rename symbols in C/C++ code.
+
+ Subcommands:
+rename-at: Perform rename off of a location in a file. (This is the default.)
+rename-all: Perform rename of all symbols matching one or more fully qualified names.
+
+
+.. code-block:: console
+
+ $ clang-rename rename-at -help
OVERVIEW: A tool to rename symbols in C/C++ code.
clang-rename renames every occurrence of a symbol found at in
. If -i is specified, the edited files are overwritten to disk.
Otherwise, the results are written to stdout.
-
- USAGE: clang-rename [subcommand] [options] [... ]
-
+
+ USAGE: clang-rename rename-at [subcommand] [options] [... ]
+
OPTIONS:
+
+ Generic Options:
+
+-help - Display available options (-help-hidden for more)
+-help-list - Display list of available options (-help-list-hidden for more)
+-version - Display the version of this program
- Clang-rename options:
+ clang-rename rename-at options:
-export-fixes= - YAML file to store suggested fixes in.
-extra-arg=- Additional argument to append to the compiler command line
-extra-arg-before= - Additional argument to prepend to the compiler command line
-i - Overwrite edited s.
-new-name= - The new name to change the symbol to.
-offset= - Locates the symbol by offset as opposed to :.
--old-name= - The fully qualified name of the symbol, if -offset is not used.
-p=- Build path
-pl- Print the locations affected by renaming to stderr.
-pn- Print the found symbol's name prior to renaming to stderr.
+
+.. code-block:: console
+
+ $
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 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#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 result doesn't seem to be too bad. I guess in a later patch it would be
> great to run clang-format on the whole clang-rename code, then all
> contributors
> could just run clang-format before committing in case LLVM coding style is
> not
> in our muscle memory. :)
Yes, but I don't think there are many locations where `clang-format` would be
triggered. I believe I ran clang-format over most parts few times.
>
>
> > 2. Only do `outs() << "abcd\n" << "efgh\n"` if you have something in
> > between, which can not be predefined. I.e. if you have ``` /// \brief Top
> > level help. static int helpMain(int argc, const char *argv[]) { errs() <<
> > "Usage: clang-rename {rename-at|rename-all} [OPTION]...\n\n" << "A tool to
> > rename symbols in C/C++ code.\n\n" << "Subcommands:\n" << " rename-at:
> > Perform rename off of a location in a file. (This is the default.)\n" << "
> > rename-all: Perform rename of all symbols matching one or more fully
> > qualified names.\n"; return 0; } ``` Just make it a const string, isn't it
> > better?
>
>
> Done. I copied llvm-cov, but I have no problems changing it.
Cool.
>
>
> > 3. `tooling::RefactoringTool` takes care of printing version IIUC, no need
> > to do that in a custom way (we don't have custom version in `clang-rename`
> > head at the moment, that was something Eugene.Zelenko sent recently).
>
>
>
Removed @; otherwise Eugene becomes subscribed :)
> Indeed, removed.
>
> > 4. `clang-rename/RenamingAction.h` -> `USRList` -> `USRs` or the other way
> > around everywhere. So far single naming convention feels right to me (I
> > personally prefer `*s` over `*List`). LLVM Coding Style
> >
> > does, too, from what I understand. Unless it's `*Set`, which is pretty
> > much different thing.
>
>
> I've changed NewNameList and PrevNameList.
>
> USRList refers to a list of lists, i.e. doing one oldname->newname rename may
> have to deal with multiple USRs, and when doing multiple oldname->newname
> renames, you need to deal with a list of list of USRs. I used the List suffix
> in this "list of list" case. I have no problem renaming
> `std::vector USRList` to USRs, but then we need a
> new
> name for `std::vector USRs`.
Aha, I see. Yes, for `vector>` it seems reasonable. Sorry, didn't that
it's `vector>`.
>
>
> > 5. Do we really need to dispatch these functions this way? with
>
> >
>
> > ``` enum RenameSubcommand { RenameAt, RenameAll }; ``` And many other
> > strange things. Just pass `bool isMultipleRename` or `isRenameAll` to
> > `main` routine instead of creating `enum`. We only have two such options
> > here, right? I pray we won't have more.
>
>
> I promise I don't plan to suggest more. :)
//looking into your honest eyes//
> Changed the enum to a bool.
>
> > 6. Move `int main(int argc, const char **argv)` upwards, so that it's above
> > dispatched routines. Otherwise when one wants to see what's going on, he
> > sees subroutine first without understanding where it comes from. Other way
> > around feels more intuitive to me.
>
>
> Done.
As for help message, look at `clang-tidy`. Is there a need in `helpMain`?
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 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 bad. I guess in a later patch it would be
great to run clang-format on the whole clang-rename code, then all contributors
could just run clang-format before committing in case LLVM coding style is not
in our muscle memory. :)
> 2. Only do `outs() << "abcd\n" << "efgh\n"` if you have something in between,
> which can not be predefined. I.e. if you have ``` /// \brief Top level help.
> static int helpMain(int argc, const char *argv[]) { errs() << "Usage:
> clang-rename {rename-at|rename-all} [OPTION]...\n\n" << "A tool to rename
> symbols in C/C++ code.\n\n" << "Subcommands:\n" << " rename-at: Perform
> rename off of a location in a file. (This is the default.)\n" << "
> rename-all: Perform rename of all symbols matching one or more fully
> qualified names.\n"; return 0; } ``` Just make it a const string, isn't it
> better?
Done. I copied llvm-cov, but I have no problems changing it.
> 3. `tooling::RefactoringTool` takes care of printing version IIUC, no need to
> do that in a custom way (we don't have custom version in `clang-rename` head
> at the moment, that was something @Eugene.Zelenko sent recently).
Indeed, removed.
> 4. `clang-rename/RenamingAction.h` -> `USRList` -> `USRs` or the other way
> around everywhere. So far single naming convention feels right to me (I
> personally prefer `*s` over `*List`). LLVM Coding Style
>
> does, too, from what I understand. Unless it's `*Set`, which is pretty much
> different thing.
I've changed NewNameList and PrevNameList.
USRList refers to a list of lists, i.e. doing one oldname->newname rename may
have to deal with multiple USRs, and when doing multiple oldname->newname
renames, you need to deal with a list of list of USRs. I used the List suffix
in this "list of list" case. I have no problem renaming
`std::vector USRList` to USRs, but then we need a new
name for `std::vector USRs`.
> 5. Do we really need to dispatch these functions this way? with
>
> ``` enum RenameSubcommand { RenameAt, RenameAll }; ``` And many other
> strange things. Just pass `bool isMultipleRename` or `isRenameAll` to `main`
> routine instead of creating `enum`. We only have two such options here,
> right? I pray we won't have more.
I promise I don't plan to suggest more. :) Changed the enum to a bool.
> 6. Move `int main(int argc, const char **argv)` upwards, so that it's above
> dispatched routines. Otherwise when one wants to see what's going on, he sees
> subroutine first without understanding where it comes from. Other way around
> feels more intuitive to me.
Done.
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 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/clang-rename/ClassTestMultiByName.cpp
test/clang-rename/FunctionWithClassFindByName.cpp
test/clang-rename/NoNewName.cpp
Index: test/clang-rename/NoNewName.cpp
===
--- test/clang-rename/NoNewName.cpp
+++ test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: ERROR: no new name provided.
+// CHECK: clang-rename: for the -new-name option: must be specified
Index: test/clang-rename/FunctionWithClassFindByName.cpp
===
--- test/clang-rename/FunctionWithClassFindByName.cpp
+++ test/clang-rename/FunctionWithClassFindByName.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-rename -old-name=Foo -new-name=Bar %s -- | FileCheck %s
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %s -- | FileCheck %s
void foo() {
}
Index: test/clang-rename/ClassTestMultiByName.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMultiByName.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-rename rename-all -old-name=Cla1 -new-name=Kla1 -old-name=Cla2 -new-name=Kla2 %s -- | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassTestMulti.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMulti.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-rename rename-all -offset=113 -new-name=Kla1 -offset=151 -new-name=Kla2 %s -- | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassFindByName.cpp
===
--- test/clang-rename/ClassFindByName.cpp
+++ test/clang-rename/ClassFindByName.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-rename -old-name=Foo -new-name=Bar %s -- | FileCheck %s
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %s -- | FileCheck %s
class Foo { // CHECK: class Bar
};
Index: docs/clang-rename.rst
===
--- docs/clang-rename.rst
+++ docs/clang-rename.rst
@@ -42,6 +42,14 @@
$ grep -FUbo 'foo' file.cpp
+You can also identify one or more symbols to be renamed by giving the fully qualified
+name:
+
+.. code-block:: console
+
+ $ clang-rename rename-all -old-name=foo -new-name=bar test.cpp
+
+
The tool currently supports renaming actions inside a single Translation Unit
only. It is planned to extend the tool's functionality to support multi-TU
renaming actions in the future.
@@ -55,34 +63,73 @@
.. code-block:: console
$ clang-rename -help
+ Usage: clang-rename {rename-at|rename-all} [OPTION]...
+
+ A tool to rename symbols in C/C++ code.
+
+ Subcommands:
+rename-at: Perform rename off of a location in a file. (This is the default.)
+rename-all: Perform rename of all symbols matching one or more fully qualified names.
+
+
+.. code-block:: console
+
+ $ clang-rename rename-at -help
OVERVIEW: A tool to rename symbols in C/C++ code.
clang-rename renames every occurrence of a symbol found at in
. If -i is specified, the edited files are overwritten to disk.
Otherwise, the results are written to stdout.
-
- USAGE: clang-rename [subcommand] [options] [... ]
-
+
+ USAGE: clang-rename rename-at [subcommand] [options] [... ]
+
OPTIONS:
+
+ Generic Options:
+
+-help - Display available options (-help-hidden for more)
+-help-list - Display list of available options (-help-list-hidden for more)
+-version - Display the version of this program
- Clang-rename options:
+ clang-rename rename-at options:
-export-fixes= - YAML file to store suggested fixes in.
-extra-arg=- Additional argument to append to the compiler command line
-extra-arg-before= - Additional argument to prepend to the compiler command line
-i - Overwrite edited s.
-new-name= - The new name to change the symbol to.
-offset= - Locates the symbol by offset as opposed to :.
--old-name= - The fully qualified name of the symbol, if -offset is not used.
-p=- Build path
-pl- Print the locations affected by renaming to stderr.
-pn- Print the found symbol's name prior to renaming to stderr.
+
+.. code-block:: console
+
+ $
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 have
/// \brief Top level help.
static int helpMain(int argc, const char *argv[]) {
errs() << "Usage: clang-rename {rename-at|rename-all} [OPTION]...\n\n"
<< "A tool to rename symbols in C/C++ code.\n\n"
<< "Subcommands:\n"
<< " rename-at: Perform rename off of a location in a file. (This
is the default.)\n"
<< " rename-all: Perform rename of all symbols matching one or more
fully qualified names.\n";
return 0;
}
Just make it a const string, isn't it better?
3. `tooling::RefactoringTool` takes care of printing version IIUC, no need to
do that in a custom way (we don't have custom version in `clang-rename` head at
the moment, that was something @Eugene.Zelenko sent recently).
4. `clang-rename/RenamingAction.h` -> `USRList` -> `USRs` or the other way
around everywhere. So far single naming convention feels right to me (I
personally prefer `*s` over `*List`). LLVM Coding Style
does, too, from what I understand. Unless it's `*Set`, which is pretty much
different thing.
5. Do we really need to dispatch these functions this way? with
enum RenameSubcommand {
RenameAt, RenameAll
};
And many other strange things. Just pass `bool isMultipleRename` or
`isRenameAll` to `main` routine instead of creating `enum`. We only have two
such options here, right? I pray we won't have more.
6. Move `int main(int argc, const char **argv)` upwards, so that it's above
dispatching routines. Otherwise when one wants to see what's going on, he sees
subroutine first without understanding where it comes from. Other way around
feels more intuitive to me.
Feel free do disagree with my points, I may be terribly wrong :)
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 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://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 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/cfe-commits
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
test/clang-rename/ClassTestMultiByName.cpp
test/clang-rename/FunctionWithClassFindByName.cpp
test/clang-rename/NoNewName.cpp
Index: test/clang-rename/NoNewName.cpp
===
--- test/clang-rename/NoNewName.cpp
+++ test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: ERROR: no new name provided.
+// CHECK: clang-rename: for the -new-name option: must be specified
Index: test/clang-rename/FunctionWithClassFindByName.cpp
===
--- test/clang-rename/FunctionWithClassFindByName.cpp
+++ test/clang-rename/FunctionWithClassFindByName.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-rename -old-name=Foo -new-name=Bar %s -- | FileCheck %s
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %s -- | FileCheck %s
void foo() {
}
Index: test/clang-rename/ClassTestMultiByName.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMultiByName.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-rename rename-all -old-name=Cla1 -new-name=Kla1 -old-name=Cla2 -new-name=Kla2 %s -- | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassTestMulti.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMulti.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-rename rename-all -offset=113 -new-name=Kla1 -offset=151 -new-name=Kla2 %s -- | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassFindByName.cpp
===
--- test/clang-rename/ClassFindByName.cpp
+++ test/clang-rename/ClassFindByName.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-rename -old-name=Foo -new-name=Bar %s -- | FileCheck %s
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %s -- | FileCheck %s
class Foo { // CHECK: class Bar
};
Index: docs/clang-rename.rst
===
--- docs/clang-rename.rst
+++ docs/clang-rename.rst
@@ -42,6 +42,14 @@
$ grep -FUbo 'foo' file.cpp
+You can also identify one or more symbols to be renamed by giving the fully qualified
+name:
+
+.. code-block:: console
+
+ $ clang-rename rename-all -old-name=foo -new-name=bar test.cpp
+
+
The tool currently supports renaming actions inside a single Translation Unit
only. It is planned to extend the tool's functionality to support multi-TU
renaming actions in the future.
@@ -55,34 +63,73 @@
.. code-block:: console
$ clang-rename -help
+ Usage: clang-rename {rename-at|rename-all} [OPTION]...
+
+ A tool to rename symbols in C/C++ code.
+
+ Subcommands:
+rename-at: Perform rename off of a location in a file. (This is the default.)
+rename-all: Perform rename of all symbols matching one or more fully qualified names.
+
+
+.. code-block:: console
+
+ $ clang-rename rename-at -help
OVERVIEW: A tool to rename symbols in C/C++ code.
clang-rename renames every occurrence of a symbol found at in
. If -i is specified, the edited files are overwritten to disk.
Otherwise, the results are written to stdout.
-
- USAGE: clang-rename [subcommand] [options] [... ]
-
+
+ USAGE: clang-rename rename-at [subcommand] [options] [... ]
+
OPTIONS:
+
+ Generic Options:
+
+-help - Display available options (-help-hidden for more)
+-help-list - Display list of available options (-help-list-hidden for more)
+-version - Display the version of this program
- Clang-rename options:
+ clang-rename rename-at options:
-export-fixes= - YAML file to store suggested fixes in.
-extra-arg=- Additional argument to append to the compiler command line
-extra-arg-before= - Additional argument to prepend to the compiler command line
-i - Overwrite edited s.
-new-name= - The new name to change the symbol to.
-offset= - Locates the symbol by offset as opposed to :.
--old-name= - The fully qualified name of the symbol, if -offset is not used.
-p=- Build path
-pl- Print the locations affected by renaming to stderr.
-pn- Print the found symbol's name prior to renaming to stderr.
+
+.. code-block:: console
+
+ $
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 clang-rename -rename-at Done. > here and later, too `clang-rename rename-at` is now only mentioned by the document when it comes to `clang-rename rename-at -help`, which is still necessary, as `clang-rename -help` talks about the subcommands only. I've updated `clang-rename -help` so that it points out that `rename-at` is the default, though. 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 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/clang-rename/ClassTestMultiByName.cpp
test/clang-rename/NoNewName.cpp
Index: test/clang-rename/NoNewName.cpp
===
--- test/clang-rename/NoNewName.cpp
+++ test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: ERROR: no new name provided.
+// CHECK: clang-rename: for the -new-name option: must be specified
Index: test/clang-rename/ClassTestMultiByName.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMultiByName.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-rename rename-all -old-name=Cla1 -new-name=Kla1 -old-name=Cla2 -new-name=Kla2 %s -- | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassTestMulti.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMulti.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-rename rename-all -offset=113 -new-name=Kla1 -offset=151 -new-name=Kla2 %s -- | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassFindByName.cpp
===
--- test/clang-rename/ClassFindByName.cpp
+++ test/clang-rename/ClassFindByName.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-rename -old-name=Foo -new-name=Bar %s -- | FileCheck %s
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %s -- | FileCheck %s
class Foo { // CHECK: class Bar
};
Index: docs/clang-rename.rst
===
--- docs/clang-rename.rst
+++ docs/clang-rename.rst
@@ -42,6 +42,14 @@
$ grep -FUbo 'foo' file.cpp
+You can also identify one or more symbols to be renamed by giving the fully qualified
+name:
+
+.. code-block:: console
+
+ $ clang-rename rename-all -old-name=foo -new-name=bar test.cpp
+
+
The tool currently supports renaming actions inside a single Translation Unit
only. It is planned to extend the tool's functionality to support multi-TU
renaming actions in the future.
@@ -55,34 +63,73 @@
.. code-block:: console
$ clang-rename -help
+ Usage: clang-rename {rename-at|rename-all} [OPTION]...
+
+ A tool to rename symbols in C/C++ code.
+
+ Subcommands:
+rename-at: Perform rename off of a location in a file. (This is the default.)
+rename-all: Perform rename of all symbols matching one or more fully qualified names.
+
+
+.. code-block:: console
+
+ $ clang-rename rename-at -help
OVERVIEW: A tool to rename symbols in C/C++ code.
clang-rename renames every occurrence of a symbol found at in
. If -i is specified, the edited files are overwritten to disk.
Otherwise, the results are written to stdout.
-
- USAGE: clang-rename [subcommand] [options] [... ]
-
+
+ USAGE: clang-rename rename-at [subcommand] [options] [... ]
+
OPTIONS:
+
+ Generic Options:
+
+-help - Display available options (-help-hidden for more)
+-help-list - Display list of available options (-help-list-hidden for more)
+-version - Display the version of this program
- Clang-rename options:
+ clang-rename rename-at options:
-export-fixes= - YAML file to store suggested fixes in.
-extra-arg=- Additional argument to append to the compiler command line
-extra-arg-before= - Additional argument to prepend to the compiler command line
-i - Overwrite edited s.
-new-name= - The new name to change the symbol to.
-offset= - Locates the symbol by offset as opposed to :.
--old-name= - The fully qualified name of the symbol, if -offset is not used.
-p=- Build path
-pl- Print the locations affected by renaming to stderr.
-pn- Print the found symbol's name prior to renaming to stderr.
+
+.. code-block:: console
+
+ $ ~/git/llvm/workdir/bin/clang-rename rename-all -help
+ OVERVIEW: A tool to rename symbols in C/C++ code.
+ clang-rename renames every occurrence of a symbol named .
+
+ USAGE: clang-rename rename-all [subcommand] [options] [... ]
+
+ OPTIONS:
+
Generic Options:
-help - Display available options (-help-hidden for more)
-help-list - Display list of available options (-help-list-hidden for more)
-version - Display
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 necessary here anymore since it's going to be default behavior IIUC Comment at: docs/clang-rename.rst:35 @@ -34,3 +34,3 @@ - $ clang-rename -offset=42 -new-name=foo test.cpp -- -Imy_project/include -DMY_DEFINES ... + $ clang-rename rename-at -offset=42 -new-name=foo test.cpp -- -Imy_project/include -DMY_DEFINES ... docs should be fixed correspondingly; i.e. prefer to write `clang-rename -offset=42` over `clang-rename -rename-at` Comment at: docs/clang-rename.rst:50 @@ +49,3 @@ + + $ clang-rename rename-all -old-name=foo -new-name=bar test.cpp + here and later, too 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 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
test/clang-rename/ClassTestMulti.cpp
test/clang-rename/ClassTestMultiByName.cpp
test/clang-rename/NoNewName.cpp
Index: test/clang-rename/NoNewName.cpp
===
--- test/clang-rename/NoNewName.cpp
+++ test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: ERROR: no new name provided.
+// CHECK: clang-rename: for the -new-name option: must be specified
Index: test/clang-rename/ClassTestMultiByName.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMultiByName.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-rename rename-all -old-name=Cla1 -new-name=Kla1 -old-name=Cla2 -new-name=Kla2 %s -- | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassTestMulti.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMulti.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-rename rename-all -offset=113 -new-name=Kla1 -offset=151 -new-name=Kla2 %s -- | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassFindByName.cpp
===
--- test/clang-rename/ClassFindByName.cpp
+++ test/clang-rename/ClassFindByName.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-rename -old-name=Foo -new-name=Bar %s -- | FileCheck %s
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %s -- | FileCheck %s
class Foo { // CHECK: class Bar
};
Index: docs/clang-rename.rst
===
--- docs/clang-rename.rst
+++ docs/clang-rename.rst
@@ -32,7 +32,7 @@
.. code-block:: console
- $ clang-rename -offset=42 -new-name=foo test.cpp -- -Imy_project/include -DMY_DEFINES ...
+ $ clang-rename rename-at -offset=42 -new-name=foo test.cpp -- -Imy_project/include -DMY_DEFINES ...
To get an offset of a symbol in a file run
@@ -42,6 +42,14 @@
$ grep -FUbo 'foo' file.cpp
+You can also identify one or more symbols to be renamed by giving the fully qualified
+name:
+
+.. code-block:: console
+
+ $ clang-rename rename-all -old-name=foo -new-name=bar test.cpp
+
+
The tool currently supports renaming actions inside a single Translation Unit
only. It is planned to extend the tool's functionality to support multi-TU
renaming actions in the future.
@@ -55,34 +63,73 @@
.. code-block:: console
$ clang-rename -help
+ Usage: clang-rename {rename-at|rename-all} [OPTION]...
+
+ A tool to rename symbols in C/C++ code.
+
+ Subcommands:
+rename-at: Perform rename off of a location in a file.
+rename-all: Perform rename of all symbols matching one or more fully qualified names.
+
+
+.. code-block:: console
+
+ $ clang-rename rename-at -help
OVERVIEW: A tool to rename symbols in C/C++ code.
clang-rename renames every occurrence of a symbol found at in
. If -i is specified, the edited files are overwritten to disk.
Otherwise, the results are written to stdout.
-
- USAGE: clang-rename [subcommand] [options] [... ]
-
+
+ USAGE: clang-rename rename-at [subcommand] [options] [... ]
+
OPTIONS:
+
+ Generic Options:
+
+-help - Display available options (-help-hidden for more)
+-help-list - Display list of available options (-help-list-hidden for more)
+-version - Display the version of this program
- Clang-rename options:
+ clang-rename rename-at options:
-export-fixes= - YAML file to store suggested fixes in.
-extra-arg=- Additional argument to append to the compiler command line
-extra-arg-before= - Additional argument to prepend to the compiler command line
-i - Overwrite edited s.
-new-name= - The new name to change the symbol to.
-offset= - Locates the symbol by offset as opposed to :.
--old-name= - The fully qualified name of the symbol, if -offset is not used.
-p=- Build path
-pl- Print the locations affected by renaming to stderr.
-pn- Print the found symbol's name prior to renaming to stderr.
+
+.. code-block:: console
+
+ $ ~/git/llvm/workdir/bin/clang-rename rename-all -help
+ OVERVIEW: A tool to rename symbols in C/C++ code.
+ clang-rename renames every occurrence of a symbol named .
+
+ USAGE: clang-rename
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
test/clang-rename/ClassTestMulti.cpp
test/clang-rename/ClassTestMultiByName.cpp
test/clang-rename/NoNewName.cpp
Index: test/clang-rename/NoNewName.cpp
===
--- test/clang-rename/NoNewName.cpp
+++ test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: ERROR: no new name provided.
+// CHECK: clang-rename: for the -new-name option: must be specified
Index: test/clang-rename/ClassTestMultiByName.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMultiByName.cpp
@@ -0,0 +1,8 @@
+// RUN: cat %s > %t.cpp
+// RUN: clang-rename rename-all -old-name=Cla1 -new-name=Kla1 -old-name=Cla2 -new-name=Kla2 %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassTestMulti.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMulti.cpp
@@ -0,0 +1,8 @@
+// RUN: cat %s > %t.cpp
+// RUN: clang-rename rename-all -offset=174 -new-name=Kla1 -offset=212 -new-name=Kla2 %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassFindByName.cpp
===
--- test/clang-rename/ClassFindByName.cpp
+++ test/clang-rename/ClassFindByName.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -old-name=Foo -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Foo { // CHECK: class Bar
Index: docs/clang-rename.rst
===
--- docs/clang-rename.rst
+++ docs/clang-rename.rst
@@ -32,7 +32,7 @@
.. code-block:: console
- $ clang-rename -offset=42 -new-name=foo test.cpp -- -Imy_project/include -DMY_DEFINES ...
+ $ clang-rename rename-at -offset=42 -new-name=foo test.cpp -- -Imy_project/include -DMY_DEFINES ...
To get an offset of a symbol in a file run
@@ -42,6 +42,14 @@
$ grep -FUbo 'foo' file.cpp
+You can also identify one or more symbols to be renamed by giving the fully qualified
+name:
+
+.. code-block:: console
+
+ $ clang-rename rename-all -old-name=foo -new-name=bar test.cpp
+
+
The tool currently supports renaming actions inside a single Translation Unit
only. It is planned to extend the tool's functionality to support multi-TU
renaming actions in the future.
@@ -55,34 +63,73 @@
.. code-block:: console
$ clang-rename -help
+ Usage: clang-rename {rename-at|rename-all} [OPTION]...
+
+ A tool to rename symbols in C/C++ code.
+
+ Subcommands:
+rename-at: Perform rename off of a location in a file.
+rename-all: Perform rename of all symbols matching one or more fully qualified names.
+
+
+.. code-block:: console
+
+ $ clang-rename rename-at -help
OVERVIEW: A tool to rename symbols in C/C++ code.
clang-rename renames every occurrence of a symbol found at in
. If -i is specified, the edited files are overwritten to disk.
Otherwise, the results are written to stdout.
-
- USAGE: clang-rename [subcommand] [options] [... ]
-
+
+ USAGE: clang-rename rename-at [subcommand] [options] [... ]
+
OPTIONS:
+
+ Generic Options:
+
+-help - Display available options (-help-hidden for more)
+-help-list - Display list of available options (-help-list-hidden for more)
+-version - Display the version of this program
- Clang-rename options:
+ clang-rename rename-at options:
-export-fixes= - YAML file to store suggested fixes in.
-extra-arg=- Additional argument to append to the compiler command line
-extra-arg-before= - Additional argument to prepend to the compiler command line
-i - Overwrite edited s.
-new-name= - The new name to change the symbol to.
-offset= - Locates the symbol by offset as opposed to :.
--old-name= - The fully qualified name of the symbol, if -offset is not used.
-p=- Build path
-pl- Print the locations affected by renaming to stderr.
-pn- Print the found symbol's name prior to renaming to stderr.
+
+.. code-block:: console
+
+ $
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
test/clang-rename/ClassTestMulti.cpp
test/clang-rename/ClassTestMultiByName.cpp
test/clang-rename/NoNewName.cpp
Index: test/clang-rename/NoNewName.cpp
===
--- test/clang-rename/NoNewName.cpp
+++ test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: ERROR: no new name provided.
+// CHECK: clang-rename: for the -new-name option: must be specified
Index: test/clang-rename/ClassTestMultiByName.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMultiByName.cpp
@@ -0,0 +1,8 @@
+// RUN: cat %s > %t.cpp
+// RUN: clang-rename rename-all -old-name=Cla1 -new-name=Kla1 -old-name=Cla2 -new-name=Kla2 %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassTestMulti.cpp
===
--- /dev/null
+++ test/clang-rename/ClassTestMulti.cpp
@@ -0,0 +1,8 @@
+// RUN: cat %s > %t.cpp
+// RUN: clang-rename rename-all -offset=174 -new-name=Kla1 -offset=212 -new-name=Kla2 %t.cpp -i --
+// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
+class Cla1 { // CHECK: class Kla1
+};
+
+class Cla2 { // CHECK: class Kla2
+};
Index: test/clang-rename/ClassFindByName.cpp
===
--- test/clang-rename/ClassFindByName.cpp
+++ test/clang-rename/ClassFindByName.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -old-name=Foo -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-all -old-name=Foo -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Foo { // CHECK: class Bar
Index: docs/clang-rename.rst
===
--- docs/clang-rename.rst
+++ docs/clang-rename.rst
@@ -32,7 +32,7 @@
.. code-block:: console
- $ clang-rename -offset=42 -new-name=foo test.cpp -- -Imy_project/include -DMY_DEFINES ...
+ $ clang-rename rename-at -offset=42 -new-name=foo test.cpp -- -Imy_project/include -DMY_DEFINES ...
To get an offset of a symbol in a file run
@@ -42,6 +42,14 @@
$ grep -FUbo 'foo' file.cpp
+You can also identify one or more symbols to be renamed by giving the fully qualified
+name:
+
+.. code-block:: console
+
+ $ clang-rename rename-all -old-name=foo -new-name=bar test.cpp
+
+
The tool currently supports renaming actions inside a single Translation Unit
only. It is planned to extend the tool's functionality to support multi-TU
renaming actions in the future.
@@ -55,34 +63,73 @@
.. code-block:: console
$ clang-rename -help
+ Usage: clang-rename {rename-at|rename-all} [OPTION]...
+
+ A tool to rename symbols in C/C++ code.
+
+ Subcommands:
+rename-at: Perform rename off of a location in a file.
+rename-all: Perform rename of all symbols matching one or more fully qualified names.
+
+
+.. code-block:: console
+
+ $ clang-rename rename-at -help
OVERVIEW: A tool to rename symbols in C/C++ code.
clang-rename renames every occurrence of a symbol found at in
. If -i is specified, the edited files are overwritten to disk.
Otherwise, the results are written to stdout.
-
- USAGE: clang-rename [subcommand] [options] [... ]
-
+
+ USAGE: clang-rename rename-at [subcommand] [options] [... ]
+
OPTIONS:
+
+ Generic Options:
+
+-help - Display available options (-help-hidden for more)
+-help-list - Display list of available options (-help-list-hidden for more)
+-version - Display the version of this program
- Clang-rename options:
+ clang-rename rename-at options:
-export-fixes= - YAML file to store suggested fixes in.
-extra-arg=- Additional argument to append to the compiler command line
-extra-arg-before= - Additional argument to prepend to the compiler command line
-i - Overwrite edited s.
-new-name= - The new name to change the symbol to.
-offset= - Locates the symbol by offset as opposed to :.
--old-name= - The fully qualified name of the symbol, if -offset is not used.
-p=- Build path
-pl- Print the locations affected by renaming to stderr.
-pn- Print the found symbol's name prior to renaming to stderr.
+
+.. code-block:: console
+
+ $
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. 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. 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, not options. "have this by default", you mean the ability to perform multiple oldname->newname replacements with one commandline invocation? If so, not allowing that when the rename-all subcommand is chosen sounds confusing to me. But perhaps I misunderstand something. ;-) 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? 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'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://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 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/clang-rename/ClassReplacements.cpp
test/clang-rename/ClassSimpleRenaming.cpp
test/clang-rename/ClassTestMulti.cpp
test/clang-rename/ClassTestMultiByName.cpp
test/clang-rename/ConstCastExpr.cpp
test/clang-rename/ConstructExpr.cpp
test/clang-rename/CtorFindByDeclaration.cpp
test/clang-rename/CtorFindByDefinition.cpp
test/clang-rename/CtorInitializer.cpp
test/clang-rename/DeclRefExpr.cpp
test/clang-rename/DtorDeclaration.cpp
test/clang-rename/DtorDefinition.cpp
test/clang-rename/DynamicCastExpr.cpp
test/clang-rename/Field.cpp
test/clang-rename/FunctionMacro.cpp
test/clang-rename/InvalidNewName.cpp
test/clang-rename/MemberExprMacro.cpp
test/clang-rename/Namespace.cpp
test/clang-rename/NoNewName.cpp
test/clang-rename/ReinterpretCastExpr.cpp
test/clang-rename/StaticCastExpr.cpp
test/clang-rename/Variable.cpp
test/clang-rename/VariableMacro.cpp
Index: test/clang-rename/VariableMacro.cpp
===
--- test/clang-rename/VariableMacro.cpp
+++ test/clang-rename/VariableMacro.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=208 -new-name=Z %t.cpp -i --
+// RUN: clang-rename rename-at -offset=218 -new-name=Z %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
#define Y X // CHECK: #define Y Z
Index: test/clang-rename/Variable.cpp
===
--- test/clang-rename/Variable.cpp
+++ test/clang-rename/Variable.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=148 -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-at -offset=158 -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
namespace A {
Index: test/clang-rename/StaticCastExpr.cpp
===
--- test/clang-rename/StaticCastExpr.cpp
+++ test/clang-rename/StaticCastExpr.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=152 -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-at -offset=162 -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Baz {
Index: test/clang-rename/ReinterpretCastExpr.cpp
===
--- test/clang-rename/ReinterpretCastExpr.cpp
+++ test/clang-rename/ReinterpretCastExpr.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=133 -new-name=X %t.cpp -i --
+// RUN: clang-rename rename-at -offset=143 -new-name=X %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Cla {
public:
Index: test/clang-rename/NoNewName.cpp
===
--- test/clang-rename/NoNewName.cpp
+++ test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
-// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: ERROR: no new name provided.
+// RUN: not clang-rename rename-at -offset=133 %s 2>&1 | FileCheck %s
+// CHECK: clang-rename rename-at: for the -new-name option: must be specified
Index: test/clang-rename/Namespace.cpp
===
--- test/clang-rename/Namespace.cpp
+++ test/clang-rename/Namespace.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=143 -new-name=llvm %t.cpp -i --
+// RUN: clang-rename rename-at -offset=153 -new-name=llvm %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
namespace foo { // CHECK: namespace llvm {
Index: test/clang-rename/MemberExprMacro.cpp
===
--- test/clang-rename/MemberExprMacro.cpp
+++ test/clang-rename/MemberExprMacro.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=156 -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-at -offset=166 -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Baz {
Index: test/clang-rename/InvalidNewName.cpp
===
--- test/clang-rename/InvalidNewName.cpp
+++ test/clang-rename/InvalidNewName.cpp
@@ -1,2 +1,2 @@
-// RUN: not clang-rename -new-name=class -offset=133 %s 2>&1 | FileCheck %s
+// RUN: not clang-rename rename-at -new-name=class -offset=133 %s 2>&1 | FileCheck %s
// CHECK: ERROR: new name is not a valid identifier in C++17.
Index: test/clang-rename/FunctionMacro.cpp
===
--- test/clang-rename/FunctionMacro.cpp
+++ test/clang-rename/FunctionMacro.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s >
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], PrevNameList[I], USRList[I]);
+}
vmiklos wrote:
> klimek wrote:
> > Question is whether if we go down that route we shouldn't do one search for
> > all the names, instead of re-looping for each name.
> You mean improving USRLocFindingASTVisitor, so that it can work with a list
> of USRs, not with just a single USR? I can do that, but if possible, I would
> like to do that as a follow-up patch; it was called in a loop already before
> this patch, so sounds orthogonal. (And it would potentially conflict with any
> pending patches that touch that class.)
>
> I could fold HandleOneRename() into HandleTranslationUnit(), but that just
> makes the resulting function larger, and does not get rid of the looping. Or
> is that what you want?
No, the former. Can you add a FIXME?
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 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 whether if we go down that route we shouldn't do one search for
> all the names, instead of re-looping for each name.
You mean improving USRLocFindingASTVisitor, so that it can work with a list of
USRs, not with just a single USR? I can do that, but if possible, I would like
to do that as a follow-up patch; it was called in a loop already before this
patch, so sounds orthogonal. (And it would potentially conflict with any
pending patches that touch that class.)
I could fold HandleOneRename() into HandleTranslationUnit(), but that just
makes the resulting function larger, and does not get rid of the looping. Or is
that what you want?
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
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 that route we shouldn't do one search for all
the names, instead of re-looping for each name.
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 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/clang-rename/ClassReplacements.cpp
test/clang-rename/ClassSimpleRenaming.cpp
test/clang-rename/ClassTestMulti.cpp
test/clang-rename/ClassTestMultiByName.cpp
test/clang-rename/ConstCastExpr.cpp
test/clang-rename/ConstructExpr.cpp
test/clang-rename/CtorFindByDeclaration.cpp
test/clang-rename/CtorFindByDefinition.cpp
test/clang-rename/CtorInitializer.cpp
test/clang-rename/DeclRefExpr.cpp
test/clang-rename/DtorDeclaration.cpp
test/clang-rename/DtorDefinition.cpp
test/clang-rename/DynamicCastExpr.cpp
test/clang-rename/Field.cpp
test/clang-rename/FunctionMacro.cpp
test/clang-rename/InvalidNewName.cpp
test/clang-rename/MemberExprMacro.cpp
test/clang-rename/Namespace.cpp
test/clang-rename/NoNewName.cpp
test/clang-rename/ReinterpretCastExpr.cpp
test/clang-rename/StaticCastExpr.cpp
test/clang-rename/Variable.cpp
test/clang-rename/VariableMacro.cpp
Index: test/clang-rename/VariableMacro.cpp
===
--- test/clang-rename/VariableMacro.cpp
+++ test/clang-rename/VariableMacro.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=208 -new-name=Z %t.cpp -i --
+// RUN: clang-rename rename-at -offset=218 -new-name=Z %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
#define Y X // CHECK: #define Y Z
Index: test/clang-rename/Variable.cpp
===
--- test/clang-rename/Variable.cpp
+++ test/clang-rename/Variable.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=148 -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-at -offset=158 -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
namespace A {
Index: test/clang-rename/StaticCastExpr.cpp
===
--- test/clang-rename/StaticCastExpr.cpp
+++ test/clang-rename/StaticCastExpr.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=152 -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-at -offset=162 -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Baz {
Index: test/clang-rename/ReinterpretCastExpr.cpp
===
--- test/clang-rename/ReinterpretCastExpr.cpp
+++ test/clang-rename/ReinterpretCastExpr.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=133 -new-name=X %t.cpp -i --
+// RUN: clang-rename rename-at -offset=143 -new-name=X %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Cla {
public:
Index: test/clang-rename/NoNewName.cpp
===
--- test/clang-rename/NoNewName.cpp
+++ test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
-// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: ERROR: no new name provided.
+// RUN: not clang-rename rename-at -offset=133 %s 2>&1 | FileCheck %s
+// CHECK: clang-rename rename-at: for the -new-name option: must be specified
Index: test/clang-rename/Namespace.cpp
===
--- test/clang-rename/Namespace.cpp
+++ test/clang-rename/Namespace.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=143 -new-name=llvm %t.cpp -i --
+// RUN: clang-rename rename-at -offset=153 -new-name=llvm %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
namespace foo { // CHECK: namespace llvm {
Index: test/clang-rename/MemberExprMacro.cpp
===
--- test/clang-rename/MemberExprMacro.cpp
+++ test/clang-rename/MemberExprMacro.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=156 -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-at -offset=166 -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Baz {
Index: test/clang-rename/InvalidNewName.cpp
===
--- test/clang-rename/InvalidNewName.cpp
+++ test/clang-rename/InvalidNewName.cpp
@@ -1,2 +1,2 @@
-// RUN: not clang-rename -new-name=class -offset=133 %s 2>&1 | FileCheck %s
+// RUN: not clang-rename rename-at -new-name=class -offset=133 %s 2>&1 | FileCheck %s
// CHECK: ERROR: new name is not a valid identifier in C++17.
Index: test/clang-rename/FunctionMacro.cpp
===
--- test/clang-rename/FunctionMacro.cpp
+++ test/clang-rename/FunctionMacro.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s >
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 logical to me to support `-offset` option in `-rename-all`, > too. OK, I've added that, with a testcase. > And introducing `-rename-all` without actually supporting multiple > renaming actions "at once" seems weird to me, too. OK, I've squashed the original diff into this one, with a testcase, which addresses this. A nice side-effect is that now the option parser takes care of checking if -new-name is provided, no need to have explicit code for that in clang-rename. > use std::function here? Done, also changed the `typedef` to `using`. 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 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/clang-rename/ClassReplacements.cpp
test/clang-rename/ClassSimpleRenaming.cpp
test/clang-rename/ClassTestMulti.cpp
test/clang-rename/ClassTestMultiByName.cpp
test/clang-rename/ConstCastExpr.cpp
test/clang-rename/ConstructExpr.cpp
test/clang-rename/CtorFindByDeclaration.cpp
test/clang-rename/CtorFindByDefinition.cpp
test/clang-rename/CtorInitializer.cpp
test/clang-rename/DeclRefExpr.cpp
test/clang-rename/DtorDeclaration.cpp
test/clang-rename/DtorDefinition.cpp
test/clang-rename/DynamicCastExpr.cpp
test/clang-rename/Field.cpp
test/clang-rename/FunctionMacro.cpp
test/clang-rename/MemberExprMacro.cpp
test/clang-rename/Namespace.cpp
test/clang-rename/NoNewName.cpp
test/clang-rename/ReinterpretCastExpr.cpp
test/clang-rename/StaticCastExpr.cpp
test/clang-rename/Variable.cpp
test/clang-rename/VariableMacro.cpp
Index: test/clang-rename/VariableMacro.cpp
===
--- test/clang-rename/VariableMacro.cpp
+++ test/clang-rename/VariableMacro.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=208 -new-name=Z %t.cpp -i --
+// RUN: clang-rename rename-at -offset=218 -new-name=Z %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
#define Y X // CHECK: #define Y Z
Index: test/clang-rename/Variable.cpp
===
--- test/clang-rename/Variable.cpp
+++ test/clang-rename/Variable.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=148 -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-at -offset=158 -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
namespace A {
Index: test/clang-rename/StaticCastExpr.cpp
===
--- test/clang-rename/StaticCastExpr.cpp
+++ test/clang-rename/StaticCastExpr.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=152 -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-at -offset=162 -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Baz {
Index: test/clang-rename/ReinterpretCastExpr.cpp
===
--- test/clang-rename/ReinterpretCastExpr.cpp
+++ test/clang-rename/ReinterpretCastExpr.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=133 -new-name=X %t.cpp -i --
+// RUN: clang-rename rename-at -offset=143 -new-name=X %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Cla {
public:
Index: test/clang-rename/NoNewName.cpp
===
--- test/clang-rename/NoNewName.cpp
+++ test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
-// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
-// CHECK: clang-rename: no new name provided.
+// RUN: not clang-rename rename-at -offset=133 %s 2>&1 | FileCheck %s
+// CHECK: clang-rename rename-at: for the -new-name option: must be specified
Index: test/clang-rename/Namespace.cpp
===
--- test/clang-rename/Namespace.cpp
+++ test/clang-rename/Namespace.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=143 -new-name=llvm %t.cpp -i --
+// RUN: clang-rename rename-at -offset=153 -new-name=llvm %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
namespace foo { // CHECK: namespace llvm {
Index: test/clang-rename/MemberExprMacro.cpp
===
--- test/clang-rename/MemberExprMacro.cpp
+++ test/clang-rename/MemberExprMacro.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=156 -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-at -offset=166 -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Baz {
Index: test/clang-rename/FunctionMacro.cpp
===
--- test/clang-rename/FunctionMacro.cpp
+++ test/clang-rename/FunctionMacro.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=199 -new-name=macro_function %t.cpp -i --
+// RUN: clang-rename rename-at -offset=209 -new-name=macro_function %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
#define moo foo // CHECK: #define moo macro_function
Index: test/clang-rename/Field.cpp
===
--- test/clang-rename/Field.cpp
+++ test/clang-rename/Field.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.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 it's
necessary at the moment; I expressed my ideas about it in comments to the other
patch. But if that's what the common use-case is... So, //TL;DR// I personally
don't see why one would want to rename multiple things at once while we still
can't rename a single symbol correctly in too many cases...
P.S. it seems logical to me to support `-offset` option in `-rename-all`, too.
And introducing `-rename-all` without actually supporting multiple renaming
actions "at once" seems weird to me, too.
Comment at: clang-rename/tool/ClangRename.cpp:226
@@ +225,3 @@
+ if (argc > 1) {
+typedef int (*MainFunction)(int, const char *[]);
+MainFunction Func = StringSwitch(argv[1])
use `std::function` here?
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
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 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#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
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` 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
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
test/clang-rename/ClassSimpleRenaming.cpp
test/clang-rename/ConstCastExpr.cpp
test/clang-rename/ConstructExpr.cpp
test/clang-rename/CtorFindByDeclaration.cpp
test/clang-rename/CtorFindByDefinition.cpp
test/clang-rename/CtorInitializer.cpp
test/clang-rename/DeclRefExpr.cpp
test/clang-rename/DtorDeclaration.cpp
test/clang-rename/DtorDefinition.cpp
test/clang-rename/DynamicCastExpr.cpp
test/clang-rename/Field.cpp
test/clang-rename/FunctionMacro.cpp
test/clang-rename/MemberExprMacro.cpp
test/clang-rename/Namespace.cpp
test/clang-rename/NoNewName.cpp
test/clang-rename/ReinterpretCastExpr.cpp
test/clang-rename/StaticCastExpr.cpp
test/clang-rename/Variable.cpp
test/clang-rename/VariableMacro.cpp
Index: test/clang-rename/VariableMacro.cpp
===
--- test/clang-rename/VariableMacro.cpp
+++ test/clang-rename/VariableMacro.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=208 -new-name=Z %t.cpp -i --
+// RUN: clang-rename rename-at -offset=218 -new-name=Z %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
#define Y X // CHECK: #define Y Z
Index: test/clang-rename/Variable.cpp
===
--- test/clang-rename/Variable.cpp
+++ test/clang-rename/Variable.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=148 -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-at -offset=158 -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
namespace A {
Index: test/clang-rename/StaticCastExpr.cpp
===
--- test/clang-rename/StaticCastExpr.cpp
+++ test/clang-rename/StaticCastExpr.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=152 -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-at -offset=162 -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Baz {
Index: test/clang-rename/ReinterpretCastExpr.cpp
===
--- test/clang-rename/ReinterpretCastExpr.cpp
+++ test/clang-rename/ReinterpretCastExpr.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=133 -new-name=X %t.cpp -i --
+// RUN: clang-rename rename-at -offset=143 -new-name=X %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Cla {
public:
Index: test/clang-rename/NoNewName.cpp
===
--- test/clang-rename/NoNewName.cpp
+++ test/clang-rename/NoNewName.cpp
@@ -1,4 +1,4 @@
// Check for an error while -new-name argument has not been passed to
// clang-rename.
-// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
+// RUN: not clang-rename rename-at -offset=133 %s 2>&1 | FileCheck %s
// CHECK: clang-rename: no new name provided.
Index: test/clang-rename/Namespace.cpp
===
--- test/clang-rename/Namespace.cpp
+++ test/clang-rename/Namespace.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=143 -new-name=llvm %t.cpp -i --
+// RUN: clang-rename rename-at -offset=153 -new-name=llvm %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
namespace foo { // CHECK: namespace llvm {
Index: test/clang-rename/MemberExprMacro.cpp
===
--- test/clang-rename/MemberExprMacro.cpp
+++ test/clang-rename/MemberExprMacro.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=156 -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-at -offset=166 -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Baz {
Index: test/clang-rename/FunctionMacro.cpp
===
--- test/clang-rename/FunctionMacro.cpp
+++ test/clang-rename/FunctionMacro.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=199 -new-name=macro_function %t.cpp -i --
+// RUN: clang-rename rename-at -offset=209 -new-name=macro_function %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
#define moo foo // CHECK: #define moo macro_function
Index: test/clang-rename/Field.cpp
===
--- test/clang-rename/Field.cpp
+++ test/clang-rename/Field.cpp
@@ -1,5 +1,5 @@
// RUN: cat %s > %t.cpp
-// RUN: clang-rename -offset=148 -new-name=Bar %t.cpp -i --
+// RUN: clang-rename rename-at -offset=158 -new-name=Bar %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
class Baz {
Index:
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 add `-rename-at` into the argument list there) seems reasonable. - Also, I'd be happy to see at least few good tests for `-rename-all` with multiple `-old-name` and `-new-name` arguments. - Why does `-rename-at` not have `-export-fixes` option anymore? - 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). 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. 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 after `llvm-cov`, that way I could break the chicken-and-egg problem I outlined in my previous comment. I've also updated all tests to use either of the two subcommands. For now I did not touch documentation, though that'll be obviously the next step, I just didn't want to make this patch larger than necessary. :-) https://reviews.llvm.org/D21814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
