[PATCH] D30493: [change-namespace] avoid adding leading '::' when possible.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL298363: [change-namespace] avoid adding leading '::' when 
possible. (authored by ioeric).

Changed prior to commit:
  https://reviews.llvm.org/D30493?vs=92475=92476#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D30493

Files:
  clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
  clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp

Index: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
===
--- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
+++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
@@ -28,6 +28,14 @@
   return Result;
 }
 
+// Given "a::b::c", returns {"a", "b", "c"}.
+llvm::SmallVector splitSymbolName(llvm::StringRef Name) {
+  llvm::SmallVector Splitted;
+  Name.split(Splitted, "::", /*MaxSplit=*/-1,
+ /*KeepEmpty=*/false);
+  return Splitted;
+}
+
 SourceLocation startLocationForType(TypeLoc TLoc) {
   // For elaborated types (e.g. `struct a::A`) we want the portion after the
   // `struct` but including the namespace qualifier, `a::`.
@@ -68,9 +76,7 @@
 return nullptr;
   const auto *CurrentContext = llvm::cast(InnerNs);
   const auto *CurrentNs = InnerNs;
-  llvm::SmallVector PartialNsNameSplitted;
-  PartialNsName.split(PartialNsNameSplitted, "::", /*MaxSplit=*/-1,
-  /*KeepEmpty=*/false);
+  auto PartialNsNameSplitted = splitSymbolName(PartialNsName);
   while (!PartialNsNameSplitted.empty()) {
 // Get the inner-most namespace in CurrentContext.
 while (CurrentContext && !llvm::isa(CurrentContext))
@@ -208,12 +214,8 @@
   if (DeclName.find(':') == llvm::StringRef::npos)
 return DeclName;
 
-  llvm::SmallVector NsNameSplitted;
-  NsName.split(NsNameSplitted, "::", /*MaxSplit=*/-1,
-   /*KeepEmpty=*/false);
-  llvm::SmallVector DeclNsSplitted;
-  DeclName.split(DeclNsSplitted, "::", /*MaxSplit=*/-1,
-   /*KeepEmpty=*/false);
+  auto NsNameSplitted = splitSymbolName(NsName);
+  auto DeclNsSplitted = splitSymbolName(DeclName);
   llvm::StringRef UnqualifiedDeclName = DeclNsSplitted.pop_back_val();
   // If the Decl is in global namespace, there is no need to shorten it.
   if (DeclNsSplitted.empty())
@@ -249,9 +251,7 @@
 std::string wrapCodeInNamespace(StringRef NestedNs, std::string Code) {
   if (Code.back() != '\n')
 Code += "\n";
-  llvm::SmallVector NsSplitted;
-  NestedNs.split(NsSplitted, "::", /*MaxSplit=*/-1,
- /*KeepEmpty=*/false);
+  auto NsSplitted = splitSymbolName(NestedNs);
   while (!NsSplitted.empty()) {
 // FIXME: consider code style for comments.
 Code = ("namespace " + NsSplitted.back() + " {\n" + Code +
@@ -282,6 +282,28 @@
   isNestedDeclContext(DeclCtx, D->getDeclContext()));
 }
 
+// Given a qualified symbol name, returns true if the symbol will be
+// incorrectly qualified without leading "::".
+bool conflictInNamespace(llvm::StringRef QualifiedSymbol,
+ llvm::StringRef Namespace) {
+  auto SymbolSplitted = splitSymbolName(QualifiedSymbol.trim(":"));
+  assert(!SymbolSplitted.empty());
+  SymbolSplitted.pop_back();  // We are only interested in namespaces.
+
+  if (SymbolSplitted.size() > 1 && !Namespace.empty()) {
+auto NsSplitted = splitSymbolName(Namespace.trim(":"));
+assert(!NsSplitted.empty());
+// We do not check the outermost namespace since it would not be a conflict
+// if it equals to the symbol's outermost namespace and the symbol name
+// would have been shortened.
+for (auto I = NsSplitted.begin() + 1, E = NsSplitted.end(); I != E; ++I) {
+  if (*I == SymbolSplitted.front())
+return true;
+}
+  }
+  return false;
+}
+
 AST_MATCHER(EnumDecl, isScoped) {
 return Node.isScoped();
 }
@@ -306,10 +328,8 @@
   OldNamespace(OldNs.ltrim(':')), NewNamespace(NewNs.ltrim(':')),
   FilePattern(FilePattern), FilePatternRE(FilePattern) {
   FileToReplacements->clear();
-  llvm::SmallVector OldNsSplitted;
-  llvm::SmallVector NewNsSplitted;
-  llvm::StringRef(OldNamespace).split(OldNsSplitted, "::");
-  llvm::StringRef(NewNamespace).split(NewNsSplitted, "::");
+  auto OldNsSplitted = splitSymbolName(OldNamespace);
+  auto NewNsSplitted = splitSymbolName(NewNamespace);
   // Calculates `DiffOldNamespace` and `DiffNewNamespace`.
   while (!OldNsSplitted.empty() && !NewNsSplitted.empty() &&
  OldNsSplitted.front() == NewNsSplitted.front()) {
@@ -825,7 +845,8 @@
 return;
   // If the reference need to be fully-qualified, add a leading "::" unless
   // NewNamespace is the global namespace.
-  if (ReplaceName == FromDeclName && !NewNamespace.empty())
+  if (ReplaceName == FromDeclName && 

[PATCH] D30493: [change-namespace] avoid adding leading '::' when possible.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 92475.
ioeric added a comment.

- fixed newly added tests.


https://reviews.llvm.org/D30493

Files:
  change-namespace/ChangeNamespace.cpp
  unittests/change-namespace/ChangeNamespaceTests.cpp

Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- unittests/change-namespace/ChangeNamespaceTests.cpp
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -242,7 +242,7 @@
  "} // namespace na\n"
  "namespace x {\n"
  "namespace y {\n"
- "class X { ::na::A a; z::Z zz; T t; };\n"
+ "class X { na::A a; z::Z zz; T t; };\n"
  "} // namespace y\n"
  "} // namespace x\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
@@ -296,8 +296,8 @@
  "namespace y {\n"
  "class C_X {\n"
  "public:\n"
- "  ::na::C_A a;\n"
- "  ::na::nc::C_C c;\n"
+ "  na::C_A a;\n"
+ "  na::nc::C_C c;\n"
  "};\n"
  "class C_Y {\n"
  "  C_X x;\n"
@@ -339,9 +339,9 @@
  "namespace x {\n"
  "namespace y {\n"
  "void f() {\n"
- "  ::na::B<::na::A> b;\n"
- "  ::na::B<::na::nc::C> b_c;\n"
- "  ::na::Two<::na::A, ::na::nc::C> two;\n"
+ "  na::B b;\n"
+ "  na::B b_c;\n"
+ "  na::Two two;\n"
  "}\n"
  "} // namespace y\n"
  "} // namespace x\n";
@@ -368,7 +368,7 @@
  "namespace y {\n"
  "\n"
  "class A {\n"
- "  ::na::nb::FWD *fwd;\n"
+ "  na::nb::FWD *fwd;\n"
  "};\n"
  "} // namespace y\n"
  "} // namespace x\n";
@@ -397,7 +397,7 @@
  "namespace y {\n"
  "\n"
  "class A {\n"
- "  ::na::nb::FWD *fwd;\n"
+ "  na::nb::FWD *fwd;\n"
  "};\n"
  "\n"
  "} // namespace y\n"
@@ -426,7 +426,7 @@
  "namespace y {\n"
  "\n"
  "class A {\n"
- "  ::na::nb::FWD *fwd;\n"
+ "  na::nb::FWD *fwd;\n"
  "};\n"
  "template class TEMP {};\n"
  "} // namespace y\n"
@@ -481,8 +481,8 @@
  "namespace x {\n"
  "namespace y {\n"
  "void fwd();\n"
- "void f(::na::C_A ca, ::na::nc::C_C cc) {\n"
- "  ::na::C_A ca_1 = ca;\n"
+ "void f(na::C_A ca, na::nc::C_C cc) {\n"
+ "  na::C_A ca_1 = ca;\n"
  "}\n"
  "} // namespace y\n"
  "} // namespace x\n";
@@ -521,9 +521,9 @@
  "namespace x {\n"
  "namespace y {\n"
  "using ::na::nc::SAME;\n"
- "using YO = ::na::nd::SAME;\n"
- "typedef ::na::nd::SAME IDENTICAL;\n"
- "void f(::na::nd::SAME Same) {}\n"
+ "using YO = na::nd::SAME;\n"
+ "typedef na::nd::SAME IDENTICAL;\n"
+ "void f(na::nd::SAME Same) {}\n"
  "} // namespace y\n"
  "} // namespace x\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
@@ -549,9 +549,9 @@
  "} // namespace na\n"
  "namespace x {\n"
  "namespace y {\n"
- "class D : public ::na::Base {\n"
+ "class D : public na::Base {\n"
  "public:\n"
- "  using AA = ::na::A; using B = ::na::Base;\n"
+ "  using AA = na::A; using B = na::Base;\n"
  "  using Base::m; using Base::Base;\n"
  "};"
  "} // namespace y\n"
@@ -596,11 +596,11 @@
   "namespace x {\n"
   "namespace y {\n"
   "class C_X {\n"
-  "  ::na::C_A na;\n"
-  "  ::na::C_A::Nested 

[PATCH] D30493: [change-namespace] avoid adding leading '::' when possible.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: change-namespace/ChangeNamespace.cpp:296
+assert(!NsSplitted.empty());
+for (auto I = NsSplitted.begin() + 1, E = NsSplitted.end(); I != E; ++I) {
+  if (*I == SymbolSplitted.front())

hokein wrote:
> ioeric wrote:
> > hokein wrote:
> > > Why skipping the first element? And use `is_contained` instead?
> > See newly added comments for reasoning.
> I see. This sounds the `conflictInNamespace` is too coupled with caller 
> because it relies on "it equals to the symbol's outermost namespace and the 
> symbol name would have been shortened" assumption. It is not straightforward 
> especially for readers who read the code at the first time.  So I'd like to 
> search from 0 (and this operation is trivial). 
This is also for correctness since it is really not a conflict when symbol and 
namespace has the same outer-most namespace. I could've dropped "the symbol 
name would have been shortened" part. 


https://reviews.llvm.org/D30493



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30493: [change-namespace] avoid adding leading '::' when possible.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

Sorry, I missed this patch.

LGTM with one nit.




Comment at: change-namespace/ChangeNamespace.cpp:296
+assert(!NsSplitted.empty());
+for (auto I = NsSplitted.begin() + 1, E = NsSplitted.end(); I != E; ++I) {
+  if (*I == SymbolSplitted.front())

ioeric wrote:
> hokein wrote:
> > Why skipping the first element? And use `is_contained` instead?
> See newly added comments for reasoning.
I see. This sounds the `conflictInNamespace` is too coupled with caller because 
it relies on "it equals to the symbol's outermost namespace and the symbol name 
would have been shortened" assumption. It is not straightforward especially for 
readers who read the code at the first time.  So I'd like to search from 0 (and 
this operation is trivial). 


https://reviews.llvm.org/D30493



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30493: [change-namespace] avoid adding leading '::' when possible.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Ping ;)


https://reviews.llvm.org/D30493



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30493: [change-namespace] avoid adding leading '::' when possible.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 92138.
ioeric marked 3 inline comments as done.
ioeric added a comment.

- Merged with origin/master
- Addressed comments. Merged with origin/master.


https://reviews.llvm.org/D30493

Files:
  change-namespace/ChangeNamespace.cpp
  unittests/change-namespace/ChangeNamespaceTests.cpp

Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- unittests/change-namespace/ChangeNamespaceTests.cpp
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -242,7 +242,7 @@
  "} // namespace na\n"
  "namespace x {\n"
  "namespace y {\n"
- "class X { ::na::A a; z::Z zz; T t; };\n"
+ "class X { na::A a; z::Z zz; T t; };\n"
  "} // namespace y\n"
  "} // namespace x\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
@@ -296,8 +296,8 @@
  "namespace y {\n"
  "class C_X {\n"
  "public:\n"
- "  ::na::C_A a;\n"
- "  ::na::nc::C_C c;\n"
+ "  na::C_A a;\n"
+ "  na::nc::C_C c;\n"
  "};\n"
  "class C_Y {\n"
  "  C_X x;\n"
@@ -339,9 +339,9 @@
  "namespace x {\n"
  "namespace y {\n"
  "void f() {\n"
- "  ::na::B<::na::A> b;\n"
- "  ::na::B<::na::nc::C> b_c;\n"
- "  ::na::Two<::na::A, ::na::nc::C> two;\n"
+ "  na::B b;\n"
+ "  na::B b_c;\n"
+ "  na::Two two;\n"
  "}\n"
  "} // namespace y\n"
  "} // namespace x\n";
@@ -368,7 +368,7 @@
  "namespace y {\n"
  "\n"
  "class A {\n"
- "  ::na::nb::FWD *fwd;\n"
+ "  na::nb::FWD *fwd;\n"
  "};\n"
  "} // namespace y\n"
  "} // namespace x\n";
@@ -397,7 +397,7 @@
  "namespace y {\n"
  "\n"
  "class A {\n"
- "  ::na::nb::FWD *fwd;\n"
+ "  na::nb::FWD *fwd;\n"
  "};\n"
  "\n"
  "} // namespace y\n"
@@ -426,7 +426,7 @@
  "namespace y {\n"
  "\n"
  "class A {\n"
- "  ::na::nb::FWD *fwd;\n"
+ "  na::nb::FWD *fwd;\n"
  "};\n"
  "template class TEMP {};\n"
  "} // namespace y\n"
@@ -481,8 +481,8 @@
  "namespace x {\n"
  "namespace y {\n"
  "void fwd();\n"
- "void f(::na::C_A ca, ::na::nc::C_C cc) {\n"
- "  ::na::C_A ca_1 = ca;\n"
+ "void f(na::C_A ca, na::nc::C_C cc) {\n"
+ "  na::C_A ca_1 = ca;\n"
  "}\n"
  "} // namespace y\n"
  "} // namespace x\n";
@@ -521,9 +521,9 @@
  "namespace x {\n"
  "namespace y {\n"
  "using ::na::nc::SAME;\n"
- "using YO = ::na::nd::SAME;\n"
- "typedef ::na::nd::SAME IDENTICAL;\n"
- "void f(::na::nd::SAME Same) {}\n"
+ "using YO = na::nd::SAME;\n"
+ "typedef na::nd::SAME IDENTICAL;\n"
+ "void f(na::nd::SAME Same) {}\n"
  "} // namespace y\n"
  "} // namespace x\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
@@ -549,9 +549,9 @@
  "} // namespace na\n"
  "namespace x {\n"
  "namespace y {\n"
- "class D : public ::na::Base {\n"
+ "class D : public na::Base {\n"
  "public:\n"
- "  using AA = ::na::A; using B = ::na::Base;\n"
+ "  using AA = na::A; using B = na::Base;\n"
  "  using Base::m; using Base::Base;\n"
  "};"
  "} // namespace y\n"
@@ -596,11 +596,11 @@
   "namespace x {\n"
   "namespace 

[PATCH] D30493: [change-namespace] avoid adding leading '::' when possible.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: change-namespace/ChangeNamespace.cpp:291
+  assert(!SymbolSplitted.empty());
+  SymbolSplitted.pop_back();
+

hokein wrote:
> Is this needed? Looks like you are removing the name of the symbol here, but 
> from the code below, you only use the first element of it.  The 
> QualifiedSymbol should always be a fully-qualified name with at least 1 
> namespace qualifier in the code, right?
`QualifiedSymbol` can be in the global namespace, so `SymbolSplitted` could be 
empty after `pop_back`.



Comment at: change-namespace/ChangeNamespace.cpp:296
+assert(!NsSplitted.empty());
+for (auto I = NsSplitted.begin() + 1, E = NsSplitted.end(); I != E; ++I) {
+  if (*I == SymbolSplitted.front())

hokein wrote:
> Why skipping the first element? And use `is_contained` instead?
See newly added comments for reasoning.


https://reviews.llvm.org/D30493



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30493: [change-namespace] avoid adding leading '::' when possible.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

I like removing the leading "::" when possible. :)




Comment at: change-namespace/ChangeNamespace.cpp:291
+  assert(!SymbolSplitted.empty());
+  SymbolSplitted.pop_back();
+

Is this needed? Looks like you are removing the name of the symbol here, but 
from the code below, you only use the first element of it.  The QualifiedSymbol 
should always be a fully-qualified name with at least 1 namespace qualifier in 
the code, right?



Comment at: change-namespace/ChangeNamespace.cpp:296
+assert(!NsSplitted.empty());
+for (auto I = NsSplitted.begin() + 1, E = NsSplitted.end(); I != E; ++I) {
+  if (*I == SymbolSplitted.front())

Why skipping the first element? And use `is_contained` instead?



Comment at: unittests/change-namespace/ChangeNamespaceTests.cpp:718
   "void f() {\n"
-  " auto *ref1 = ::na::A::f;\n"
-  " auto *ref2 = ::na::a_f;\n"
-  " auto *ref3 = ::na::s_f;\n"
+  " auto *ref1 = na::A::f;\n"
+  " auto *ref2 = na::a_f;\n"

The code style is not correct. Also fix it.


https://reviews.llvm.org/D30493



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30493: [change-namespace] avoid adding leading '::' when possible.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.

When changing namespaces, the tool adds leading "::" to references that need to
be fully-qualified, which would affect readability.

We avoid adding "::" when the symbol name does not conflict with the new
namespace name. For example, a symbol name "na::nb::X" conflicts with "ns::na"
since it would be resolved to "ns::na::nb::X" in the new namespace.


https://reviews.llvm.org/D30493

Files:
  change-namespace/ChangeNamespace.cpp
  unittests/change-namespace/ChangeNamespaceTests.cpp

Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- unittests/change-namespace/ChangeNamespaceTests.cpp
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -242,7 +242,7 @@
  "} // namespace na\n"
  "namespace x {\n"
  "namespace y {\n"
- "class X { ::na::A a; z::Z zz; T t; };\n"
+ "class X { na::A a; z::Z zz; T t; };\n"
  "} // namespace y\n"
  "} // namespace x\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
@@ -296,8 +296,8 @@
  "namespace y {\n"
  "class C_X {\n"
  "public:\n"
- "  ::na::C_A a;\n"
- "  ::na::nc::C_C c;\n"
+ "  na::C_A a;\n"
+ "  na::nc::C_C c;\n"
  "};\n"
  "class C_Y {\n"
  "  C_X x;\n"
@@ -339,9 +339,9 @@
  "namespace x {\n"
  "namespace y {\n"
  "void f() {\n"
- "  ::na::B<::na::A> b;\n"
- "  ::na::B<::na::nc::C> b_c;\n"
- "  ::na::Two<::na::A, ::na::nc::C> two;\n"
+ "  na::B b;\n"
+ "  na::B b_c;\n"
+ "  na::Two two;\n"
  "}\n"
  "} // namespace y\n"
  "} // namespace x\n";
@@ -368,7 +368,7 @@
  "namespace y {\n"
  "\n"
  "class A {\n"
- "  ::na::nb::FWD *fwd;\n"
+ "  na::nb::FWD *fwd;\n"
  "};\n"
  "} // namespace y\n"
  "} // namespace x\n";
@@ -397,7 +397,7 @@
  "namespace y {\n"
  "\n"
  "class A {\n"
- "  ::na::nb::FWD *fwd;\n"
+ "  na::nb::FWD *fwd;\n"
  "};\n"
  "\n"
  "} // namespace y\n"
@@ -426,7 +426,7 @@
  "namespace y {\n"
  "\n"
  "class A {\n"
- "  ::na::nb::FWD *fwd;\n"
+ "  na::nb::FWD *fwd;\n"
  "};\n"
  "template class TEMP {};\n"
  "} // namespace y\n"
@@ -481,8 +481,8 @@
  "namespace x {\n"
  "namespace y {\n"
  "void fwd();\n"
- "void f(::na::C_A ca, ::na::nc::C_C cc) {\n"
- "  ::na::C_A ca_1 = ca;\n"
+ "void f(na::C_A ca, na::nc::C_C cc) {\n"
+ "  na::C_A ca_1 = ca;\n"
  "}\n"
  "} // namespace y\n"
  "} // namespace x\n";
@@ -521,9 +521,9 @@
  "namespace x {\n"
  "namespace y {\n"
  "using ::na::nc::SAME;\n"
- "using YO = ::na::nd::SAME;\n"
- "typedef ::na::nd::SAME IDENTICAL;\n"
- "void f(::na::nd::SAME Same) {}\n"
+ "using YO = na::nd::SAME;\n"
+ "typedef na::nd::SAME IDENTICAL;\n"
+ "void f(na::nd::SAME Same) {}\n"
  "} // namespace y\n"
  "} // namespace x\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
@@ -549,9 +549,9 @@
  "} // namespace na\n"
  "namespace x {\n"
  "namespace y {\n"
- "class D : public ::na::Base {\n"
+ "class D : public na::Base {\n"
  "public:\n"
- "  using AA = ::na::A; using B = ::na::Base;\n"
+ "  using AA = na::A; using B = na::Base;\n"