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

Summary:
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.

Reviewers: hokein

Reviewed By: hokein

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D30493

llvm-svn: 298363

2 files changed, 124 insertions, 62 deletions

diff --git a/clang-tools-extra/change-namespace/ChangeNamespace.cpp b/clang-tools-extra/change-namespace/ChangeNamespace.cpp
index e07066794b5..0b90de414c5 100644
--- a/clang-tools-extra/change-namespace/ChangeNamespace.cpp
+++ b/clang-tools-extra/change-namespace/ChangeNamespace.cpp
@@ -28,6 +28,14 @@ joinNamespaces(const llvm::SmallVectorImpl<StringRef> &Namespaces) {
   return Result;
 }
 
+// Given "a::b::c", returns {"a", "b", "c"}.
+llvm::SmallVector<llvm::StringRef, 4> splitSymbolName(llvm::StringRef Name) {
+  llvm::SmallVector<llvm::StringRef, 4> 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 @@ const NamespaceDecl *getOuterNamespace(const NamespaceDecl *InnerNs,
     return nullptr;
   const auto *CurrentContext = llvm::cast<DeclContext>(InnerNs);
   const auto *CurrentNs = InnerNs;
-  llvm::SmallVector<llvm::StringRef, 4> 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<NamespaceDecl>(CurrentContext))
@@ -208,12 +214,8 @@ std::string getShortestQualifiedNameInNamespace(llvm::StringRef DeclName,
   if (DeclName.find(':') == llvm::StringRef::npos)
     return DeclName;
 
-  llvm::SmallVector<llvm::StringRef, 4> NsNameSplitted;
-  NsName.split(NsNameSplitted, "::", /*MaxSplit=*/-1,
-               /*KeepEmpty=*/false);
-  llvm::SmallVector<llvm::StringRef, 4> 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 getShortestQualifiedNameInNamespace(llvm::StringRef DeclName,
 std::string wrapCodeInNamespace(StringRef NestedNs, std::string Code) {
   if (Code.back() != '\n')
     Code += "\n";
-  llvm::SmallVector<StringRef, 4> 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 @@ bool isDeclVisibleAtLocation(const SourceManager &SM, const Decl *D,
           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 @@ ChangeNamespaceTool::ChangeNamespaceTool(
       OldNamespace(OldNs.ltrim(':')), NewNamespace(NewNs.ltrim(':')),
       FilePattern(FilePattern), FilePatternRE(FilePattern) {
   FileToReplacements->clear();
-  llvm::SmallVector<llvm::StringRef, 4> OldNsSplitted;
-  llvm::SmallVector<llvm::StringRef, 4> 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 @@ void ChangeNamespaceTool::replaceQualifiedSymbolInDeclContext(
     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 && !NewNamespace.empty() &&
+      conflictInNamespace(ReplaceName, NewNamespace))
     ReplaceName = "::" + ReplaceName;
   addReplacementOrDie(Start, End, ReplaceName, *Result.SourceManager,
                       &FileToReplacements);
diff --git a/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp b/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp
index a5e60eaaee4..dc8eeb749c1 100644
--- a/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp
+++ b/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -242,7 +242,7 @@ TEST_F(ChangeNamespaceTest, MoveIntoExistingNamespaceAndShortenRefs) {
                          "} // 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 @@ TEST_F(ChangeNamespaceTest, SimpleMoveWithTypeRefs) {
                          "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 @@ TEST_F(ChangeNamespaceTest, TypeLocInTemplateSpecialization) {
                          "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<na::A> b;\n"
+                         "  na::B<na::nc::C> b_c;\n"
+                         "  na::Two<na::A, na::nc::C> two;\n"
                          "}\n"
                          "} // namespace y\n"
                          "} // namespace x\n";
@@ -368,7 +368,7 @@ TEST_F(ChangeNamespaceTest, LeaveForwardDeclarationBehind) {
                          "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 @@ TEST_F(ChangeNamespaceTest, InsertForwardDeclsProperly) {
                          "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 @@ TEST_F(ChangeNamespaceTest, TemplateClassForwardDeclaration) {
                          "namespace y {\n"
                          "\n"
                          "class A {\n"
-                         "  ::na::nb::FWD *fwd;\n"
+                         "  na::nb::FWD *fwd;\n"
                          "};\n"
                          "template<typename T> class TEMP {};\n"
                          "} // namespace y\n"
@@ -481,8 +481,8 @@ TEST_F(ChangeNamespaceTest, MoveFunctions) {
                          "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 @@ TEST_F(ChangeNamespaceTest, FixUsingShadowDecl) {
                          "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 @@ TEST_F(ChangeNamespaceTest, DontFixUsingShadowDeclInClasses) {
                          "} // 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 @@ TEST_F(ChangeNamespaceTest, TypeInNestedNameSpecifier) {
       "namespace x {\n"
       "namespace y {\n"
       "class C_X {\n"
-      "  ::na::C_A na;\n"
-      "  ::na::C_A::Nested nested;\n"
+      "  na::C_A na;\n"
+      "  na::C_A::Nested nested;\n"
       "  void f() {\n"
-      "    ::na::C_A::Nested::nestedFunc();\n"
-      "    int X = ::na::C_A::Nested::NestedX;\n"
+      "    na::C_A::Nested::nestedFunc();\n"
+      "    int X = na::C_A::Nested::NestedX;\n"
       "  }\n"
       "};\n"
       "}  // namespace y\n"
@@ -638,8 +638,8 @@ TEST_F(ChangeNamespaceTest, FixFunctionNameSpecifiers) {
       "}  // namespace na\n"
       "namespace x {\n"
       "namespace y {\n"
-      "void f() { ::na::a_f(); ::na::static_f(); ::na::A::f(); }\n"
-      "void g() { f(); ::na::A::g(); }\n"
+      "void f() { na::a_f(); na::static_f(); na::A::f(); }\n"
+      "void g() { f(); na::A::g(); }\n"
       "}  // namespace y\n"
       "}  // namespace x\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
@@ -675,8 +675,8 @@ TEST_F(ChangeNamespaceTest, FixOverloadedOperatorFunctionNameSpecifiers) {
       "namespace x {\n"
       "namespace y {\n"
       "bool f() {\n"
-      "  ::na::A x, y;\n"
-      "  auto f = ::na::operator<;\n"
+      "  na::A x, y;\n"
+      "  auto f = na::operator<;\n"
       // FIXME: function calls to overloaded operators are not fixed now even if
       // they are referenced by qualified names.
       "  return (x == y) && (x < y) && (operator<(x,y));\n"
@@ -715,9 +715,9 @@ TEST_F(ChangeNamespaceTest, FixNonCallingFunctionReferences) {
       "namespace x {\n"
       "namespace y {\n"
       "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"
+      "  auto *ref3 = na::s_f;\n"
       "}\n"
       "}  // namespace y\n"
       "}  // namespace x\n";
@@ -749,9 +749,9 @@ TEST_F(ChangeNamespaceTest, MoveAndFixGlobalVariables) {
                          "namespace y {\n"
                          "int GlobB;\n"
                          "void f() {\n"
-                         "  int a = ::na::GlobA;\n"
-                         "  int b = ::na::GlobAStatic;\n"
-                         "  int c = ::na::nc::GlobC;\n"
+                         "  int a = na::GlobA;\n"
+                         "  int b = na::GlobAStatic;\n"
+                         "  int c = na::nc::GlobC;\n"
                          "}\n"
                          "}  // namespace y\n"
                          "}  // namespace x\n";
@@ -786,7 +786,7 @@ TEST_F(ChangeNamespaceTest, DoNotFixStaticVariableOfClass) {
                          "namespace x {\n"
                          "namespace y {\n"
                          "void f() {\n"
-                         "  int a = ::na::A::A1; int b = ::na::A::A2;\n"
+                         "  int a = na::A::A1; int b = na::A::A2;\n"
                          "}\n"
                          "}  // namespace y\n"
                          "}  // namespace x\n";
@@ -901,7 +901,7 @@ TEST_F(ChangeNamespaceTest, NamespaceAliasInGlobal) {
                      "}\n"
                      "namespace glob2 { class Glob2 {}; }\n"
                      "namespace gl = glob;\n"
-                     "namespace gl2 = ::glob2;\n"
+                     "namespace gl2 = glob2;\n"
                      "namespace na {\n"
                      "namespace nb {\n"
                      "void f() { gl::Glob g; gl2::Glob2 g2; }\n"
@@ -914,7 +914,7 @@ TEST_F(ChangeNamespaceTest, NamespaceAliasInGlobal) {
       "}\n"
       "namespace glob2 { class Glob2 {}; }\n"
       "namespace gl = glob;\n"
-      "namespace gl2 = ::glob2;\n"
+      "namespace gl2 = glob2;\n"
       "\n"
       "namespace x {\n"
       "namespace y {\n"
@@ -1227,7 +1227,7 @@ TEST_F(ChangeNamespaceTest, UsingDeclInMovedNamespace) {
                          "} // na\n"
                          "namespace x {\n"
                          "namespace y {\n"
-                         "void d() { ::nx::f(); }\n"
+                         "void d() { nx::f(); }\n"
                          "} // namespace y\n"
                          "} // namespace x\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
@@ -1278,7 +1278,7 @@ TEST_F(ChangeNamespaceTest, UsingDeclInMovedNamespaceMultiNested) {
                          "} // c\n"
                          "namespace x {\n"
                          "namespace y {\n"
-                         "void d() { f(); ::nx::g(); }\n"
+                         "void d() { f(); nx::g(); }\n"
                          "} // namespace y\n"
                          "} // namespace x\n"
                          "} // b\n"
@@ -1521,7 +1521,7 @@ TEST_F(ChangeNamespaceTest, DerivedClassWithConstructorsAndTypeRefs) {
       "  A() : X(0) {}\n"
       "  A(int i);\n"
       "};\n"
-      "A::A(int i) : X(i) { ::nx::ny::X x(1);}\n"
+      "A::A(int i) : X(i) { nx::ny::X x(1);}\n"
       "} // namespace y\n"
       "} // namespace x\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
@@ -1595,9 +1595,9 @@ TEST_F(ChangeNamespaceTest, KeepGlobalSpecifier) {
                          "public:\n"
                          "  ::Glob glob_1;\n"
                          "  Glob glob_2;\n"
-                         "  ::na::C_A a_1;\n"
+                         "  na::C_A a_1;\n"
                          "  ::na::C_A a_2;\n"
-                         "  ::na::nc::C_C c;\n"
+                         "  na::nc::C_C c;\n"
                          "};\n"
                          "} // namespace y\n"
                          "} // namespace x\n";
@@ -1731,6 +1731,47 @@ TEST_F(ChangeNamespaceTest, ExistingNamespaceConflictWithNewNamespace) {
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, SymbolConflictWithNewNamespace) {
+  OldNamespace = "nx";
+  NewNamespace = "ny::na::nc";
+  std::string Code = "namespace na {\n"
+                     "class A {};\n"
+                     "namespace nb {\n"
+                     "class B {};\n"
+                     "} // namespace nb\n"
+                     "} // namespace na\n"
+                     "namespace ny {\n"
+                     "class Y {};\n"
+                     "}\n"
+                     "namespace nx {\n"
+                     "class X {\n"
+                     "  na::A a; na::nb::B b;\n"
+                     "  ny::Y y;"
+                     "};\n"
+                     "} // namespace nx\n";
+  std::string Expected = "namespace na {\n"
+                         "class A {};\n"
+                         "namespace nb {\n"
+                         "class B {};\n"
+                         "} // namespace nb\n"
+                         "} // namespace na\n"
+                         "namespace ny {\n"
+                         "class Y {};\n"
+                         "}\n"
+                         "\n"
+                         "namespace ny {\n"
+                         "namespace na {\n"
+                         "namespace nc {\n"
+                         "class X {\n"
+                         "  ::na::A a; ::na::nb::B b;\n"
+                         "  Y y;\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";
@@ -1848,9 +1889,9 @@ TEST_F(ChangeNamespaceTest, ReferencesToEnums) {
                          "void f() {\n"
                          "  Glob g1 = Glob::G1;\n"
                          "  Glob g2 = G2;\n"
-                         "  ::na::X x1 = ::na::X::X1;\n"
-                         "  ::na::Y y1 = ::na::Y::Y1;\n"
-                         "  ::na::Y y2 = ::na::Y::Y2;\n"
+                         "  na::X x1 = na::X::X1;\n"
+                         "  na::Y y1 = na::Y::Y1;\n"
+                         "  na::Y y2 = na::Y::Y2;\n"
                          "}\n"
                          "} // namespace y\n"
                          "} // namespace x\n";
@@ -1883,7 +1924,7 @@ TEST_F(ChangeNamespaceTest, NoRedundantEnumUpdate) {
                          "  ns::X x1 = ns::X::X1;\n"
                          "  ns::Y y1 = ns::Y::Y1;\n"
                          // FIXME: this is redundant
-                         "  ns::Y y2 = ::ns::Y::Y2;\n"
+                         "  ns::Y y2 = ns::Y::Y2;\n"
                          "}\n"
                          "} // namespace y\n"
                          "} // namespace x\n";
@@ -1997,8 +2038,8 @@ TEST_F(ChangeNamespaceTest, EnumInClass) {
                          "namespace x {\n"
                          "namespace y {\n"
                          "void f() {\n"
-                         "  ::na::X::E e = ::na::X::E1;\n"
-                         "  ::na::X::E ee = ::na::X::E::E1;\n"
+                         "  na::X::E e = na::X::E1;\n"
+                         "  na::X::E ee = na::X::E::E1;\n"
                          "}\n"
                          "} // namespace y\n"
                          "} // namespace x\n";
@@ -2043,7 +2084,7 @@ TEST_F(ChangeNamespaceTest, TypeAsTemplateParameter) {
                          "  TempTemp(t);\n"
                          "}\n"
                          "void f() {\n"
-                         "  ::na::X x;\n"
+                         "  na::X x;\n"
                          "  Temp(x);\n"
                          "}\n"
                          "} // namespace y\n"