[PATCH] D29176: [change-namespace] add leading '::' to references in new namespace when name conflict is possible.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL293182: [change-namespace] add leading '::' to references in 
new namespace when name… (authored by ioeric).

Changed prior to commit:
  https://reviews.llvm.org/D29176?vs=85903=85904#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D29176

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

Index: clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
+++ clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -1599,6 +1599,74 @@
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, ExistingNamespaceConflictWithNewNamespace) {
+  OldNamespace = "nx";
+  NewNamespace = "ny::na::nc";
+  std::string Code = "namespace na {\n"
+ "class A {};\n"
+ "} // namespace na\n"
+ "namespace nb {\n"
+ "class B {};\n"
+ "} // namespace nb\n"
+ "namespace nx {\n"
+ "class X {\n"
+ " na::A a; nb::B b;\n"
+ "};\n"
+ "} // namespace nx\n";
+  std::string Expected = "namespace na {\n"
+ "class A {};\n"
+ "} // namespace na\n"
+ "namespace nb {\n"
+ "class B {};\n"
+ "} // namespace nb\n"
+ "\n"
+ "namespace ny {\n"
+ "namespace na {\n"
+ "namespace nc {\n"
+ "class X {\n"
+ " ::na::A a; nb::B b;\n"
+ "};\n"
+ "} // namespace nc\n"
+ "} // namespace na\n"
+ "} // namespace ny\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, ShortenNamespaceSpecifier) {
+  OldNamespace = "nx";
+  NewNamespace = "ny::na";
+  std::string Code = "class G {};\n"
+ "namespace ny {\n"
+ "class Y {};\n"
+ "namespace na {\n"
+ "class A {};\n"
+ "namespace nc { class C {}; } // namespace nc\n"
+ "}\n // namespace na\n"
+ "}\n // namespace ny\n"
+ "namespace nx {\n"
+ "class X {\n"
+ " G g; ny::Y y; ny::na::A a; ny::na::nc::C c;\n"
+ "};\n"
+ "} // namespace nx\n";
+  std::string Expected = "class G {};\n"
+ "namespace ny {\n"
+ "class Y {};\n"
+ "namespace na {\n"
+ "class A {};\n"
+ "namespace nc { class C {}; } // namespace nc\n"
+ "}\n // namespace na\n"
+ "}\n // namespace ny\n"
+ "\n"
+ "namespace ny {\n"
+ "namespace na {\n"
+ "class X {\n"
+ " G g; Y y; A a; nc::C c;\n"
+ "};\n"
+ "} // namespace na\n"
+ "} // namespace ny\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
 } // anonymous namespace
 } // namespace change_namespace
 } // namespace clang
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
@@ -196,6 +196,8 @@
 // Returns the shortest qualified name for declaration `DeclName` in the
 // namespace `NsName`. For example, if `DeclName` is "a::b::X" and `NsName`
 // is "a::c::d", then "b::X" will be returned.
+// Note that if `DeclName` is `::b::X` and `NsName` is `::a::b`, this returns
+// "::b::X" instead of "b::X" since there will be a name conflict otherwise.
 // \param DeclName A fully qualified name, "::a::b::X" or "a::b::X".
 // \param NsName A fully qualified name, "::a::b" or "a::b". Global namespace
 //will have empty name.
@@ -206,14 +208,42 @@
   if (DeclName.find(':') == llvm::StringRef::npos)
 return DeclName;
 
-  while (!DeclName.consume_front((NsName + "::").str())) {
-const auto Pos = NsName.find_last_of(':');
-if (Pos == llvm::StringRef::npos)
-  return DeclName;
-assert(Pos > 0);
-NsName = NsName.substr(0, Pos - 1);
+  

[PATCH] D29176: [change-namespace] add leading '::' to references in new namespace when name conflict is possible.

[Eric Liu via Phabricator via cfe-commits]

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

- Addressed comments.


https://reviews.llvm.org/D29176

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
@@ -1599,6 +1599,74 @@
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, ExistingNamespaceConflictWithNewNamespace) {
+  OldNamespace = "nx";
+  NewNamespace = "ny::na::nc";
+  std::string Code = "namespace na {\n"
+ "class A {};\n"
+ "} // namespace na\n"
+ "namespace nb {\n"
+ "class B {};\n"
+ "} // namespace nb\n"
+ "namespace nx {\n"
+ "class X {\n"
+ " na::A a; nb::B b;\n"
+ "};\n"
+ "} // namespace nx\n";
+  std::string Expected = "namespace na {\n"
+ "class A {};\n"
+ "} // namespace na\n"
+ "namespace nb {\n"
+ "class B {};\n"
+ "} // namespace nb\n"
+ "\n"
+ "namespace ny {\n"
+ "namespace na {\n"
+ "namespace nc {\n"
+ "class X {\n"
+ " ::na::A a; nb::B b;\n"
+ "};\n"
+ "} // namespace nc\n"
+ "} // namespace na\n"
+ "} // namespace ny\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, ShortenNamespaceSpecifier) {
+  OldNamespace = "nx";
+  NewNamespace = "ny::na";
+  std::string Code = "class G {};\n"
+ "namespace ny {\n"
+ "class Y {};\n"
+ "namespace na {\n"
+ "class A {};\n"
+ "namespace nc { class C {}; } // namespace nc\n"
+ "}\n // namespace na\n"
+ "}\n // namespace ny\n"
+ "namespace nx {\n"
+ "class X {\n"
+ " G g; ny::Y y; ny::na::A a; ny::na::nc::C c;\n"
+ "};\n"
+ "} // namespace nx\n";
+  std::string Expected = "class G {};\n"
+ "namespace ny {\n"
+ "class Y {};\n"
+ "namespace na {\n"
+ "class A {};\n"
+ "namespace nc { class C {}; } // namespace nc\n"
+ "}\n // namespace na\n"
+ "}\n // namespace ny\n"
+ "\n"
+ "namespace ny {\n"
+ "namespace na {\n"
+ "class X {\n"
+ " G g; Y y; A a; nc::C c;\n"
+ "};\n"
+ "} // namespace na\n"
+ "} // namespace ny\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
 } // anonymous namespace
 } // namespace change_namespace
 } // namespace clang
Index: change-namespace/ChangeNamespace.cpp
===
--- change-namespace/ChangeNamespace.cpp
+++ change-namespace/ChangeNamespace.cpp
@@ -196,6 +196,8 @@
 // Returns the shortest qualified name for declaration `DeclName` in the
 // namespace `NsName`. For example, if `DeclName` is "a::b::X" and `NsName`
 // is "a::c::d", then "b::X" will be returned.
+// Note that if `DeclName` is `::b::X` and `NsName` is `::a::b`, this returns
+// "::b::X" instead of "b::X" since there will be a name conflict otherwise.
 // \param DeclName A fully qualified name, "::a::b::X" or "a::b::X".
 // \param NsName A fully qualified name, "::a::b" or "a::b". Global namespace
 //will have empty name.
@@ -206,14 +208,42 @@
   if (DeclName.find(':') == llvm::StringRef::npos)
 return DeclName;
 
-  while (!DeclName.consume_front((NsName + "::").str())) {
-const auto Pos = NsName.find_last_of(':');
-if (Pos == llvm::StringRef::npos)
-  return DeclName;
-assert(Pos > 0);
-NsName = NsName.substr(0, Pos - 1);
+  llvm::SmallVector NsNameSplitted;
+  NsName.split(NsNameSplitted, "::", /*MaxSplit=*/-1,
+   /*KeepEmpty=*/false);
+  llvm::SmallVector DeclNsSplitted;
+  DeclName.split(DeclNsSplitted, "::", /*MaxSplit=*/-1,
+   /*KeepEmpty=*/false);
+  llvm::StringRef UnqualifiedDeclName = DeclNsSplitted.pop_back_val();
+  // If the Decl 

[PATCH] D29176: [change-namespace] add leading '::' to references in new namespace when name conflict is possible.

[Benjamin Kramer via Phabricator via cfe-commits]

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

Only nits below.




Comment at: change-namespace/ChangeNamespace.cpp:232
+auto  = DeclNsSplitted.front();
+for (auto  : NsNameSplitted)
+  if (Ns == DeclNsTop)

This could use llvm::is_contained



Comment at: change-namespace/ChangeNamespace.cpp:237
+  }
+  // Since there is already an overlap namespace, we know that `DeclName` can 
be
+  // shortened.

I guess you could add here that you're now searching the longest common prefix.



Comment at: change-namespace/ChangeNamespace.cpp:243
+  auto NsE = NsNameSplitted.end();
+  for (; DeclI != DeclE && NsI != NsE && *DeclI == *NsI; ++DeclI, ++NsI) {
   }

Not sure if this should be a while loop. It's so ugly right now :(


https://reviews.llvm.org/D29176



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

[PATCH] D29176: [change-namespace] add leading '::' to references in new namespace when name conflict is possible.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.

For example, when we change 'na' to "nb::nc", we need to add leading '::' to
references "::nc::X" in the changed namespace.


https://reviews.llvm.org/D29176

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
@@ -1599,6 +1599,74 @@
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, ExistingNamespaceConflictWithNewNamespace) {
+  OldNamespace = "nx";
+  NewNamespace = "ny::na::nc";
+  std::string Code = "namespace na {\n"
+ "class A {};\n"
+ "} // namespace na\n"
+ "namespace nb {\n"
+ "class B {};\n"
+ "} // namespace nb\n"
+ "namespace nx {\n"
+ "class X {\n"
+ " na::A a; nb::B b;\n"
+ "};\n"
+ "} // namespace nx\n";
+  std::string Expected = "namespace na {\n"
+ "class A {};\n"
+ "} // namespace na\n"
+ "namespace nb {\n"
+ "class B {};\n"
+ "} // namespace nb\n"
+ "\n"
+ "namespace ny {\n"
+ "namespace na {\n"
+ "namespace nc {\n"
+ "class X {\n"
+ " ::na::A a; nb::B b;\n"
+ "};\n"
+ "} // namespace nc\n"
+ "} // namespace na\n"
+ "} // namespace ny\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, ShortenNamespaceSpecifier) {
+  OldNamespace = "nx";
+  NewNamespace = "ny::na";
+  std::string Code = "class G {};\n"
+ "namespace ny {\n"
+ "class Y {};\n"
+ "namespace na {\n"
+ "class A {};\n"
+ "namespace nc { class C {}; } // namespace nc\n"
+ "}\n // namespace na\n"
+ "}\n // namespace ny\n"
+ "namespace nx {\n"
+ "class X {\n"
+ " G g; ny::Y y; ny::na::A a; ny::na::nc::C c;\n"
+ "};\n"
+ "} // namespace nx\n";
+  std::string Expected = "class G {};\n"
+ "namespace ny {\n"
+ "class Y {};\n"
+ "namespace na {\n"
+ "class A {};\n"
+ "namespace nc { class C {}; } // namespace nc\n"
+ "}\n // namespace na\n"
+ "}\n // namespace ny\n"
+ "\n"
+ "namespace ny {\n"
+ "namespace na {\n"
+ "class X {\n"
+ " G g; Y y; A a; nc::C c;\n"
+ "};\n"
+ "} // namespace na\n"
+ "} // namespace ny\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
 } // anonymous namespace
 } // namespace change_namespace
 } // namespace clang
Index: change-namespace/ChangeNamespace.cpp
===
--- change-namespace/ChangeNamespace.cpp
+++ change-namespace/ChangeNamespace.cpp
@@ -196,6 +196,8 @@
 // Returns the shortest qualified name for declaration `DeclName` in the
 // namespace `NsName`. For example, if `DeclName` is "a::b::X" and `NsName`
 // is "a::c::d", then "b::X" will be returned.
+// Note that if `DeclName` is `::b::X` and `NsName` is `::a::b`, this returns
+// "::b::X" instead of "b::X" since there will be a name conflict otherwise.
 // \param DeclName A fully qualified name, "::a::b::X" or "a::b::X".
 // \param NsName A fully qualified name, "::a::b" or "a::b". Global namespace
 //will have empty name.
@@ -206,14 +208,44 @@
   if (DeclName.find(':') == llvm::StringRef::npos)
 return DeclName;
 
-  while (!DeclName.consume_front((NsName + "::").str())) {
-const auto Pos = NsName.find_last_of(':');
-if (Pos == llvm::StringRef::npos)
-  return DeclName;
-assert(Pos > 0);
-NsName = NsName.substr(0, Pos - 1);
+  llvm::SmallVector NsNameSplitted;
+  NsName.split(NsNameSplitted, "::", /*MaxSplit=*/-1,
+   /*KeepEmpty=*/false);
+  llvm::SmallVector DeclNsSplitted;
+  DeclName.split(DeclNsSplitted, "::", /*MaxSplit=*/-1,
+   /*KeepEmpty=*/false);
+  llvm::StringRef UnqualifiedDeclName =