From 8bc241641425cb306d130f3402f56f873f61b302 Mon Sep 17 00:00:00 2001 From: Eric Liu Date: Tue, 21 Mar 2017 12:41:59 +0000 Subject: [PATCH] [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 --- .../change-namespace/ChangeNamespace.cpp | 55 +++++--- .../change-namespace/ChangeNamespaceTests.cpp | 131 ++++++++++++------ 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 e07066794b51..0b90de414c54 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 &Namespaces) { 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 @@ const NamespaceDecl *getOuterNamespace(const NamespaceDecl *InnerNs, 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 @@ std::string getShortestQualifiedNameInNamespace(llvm::StringRef DeclName, 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 getShortestQualifiedNameInNamespace(llvm::StringRef DeclName, 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 @@ 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 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 @@ 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 a5e60eaaee4d..dc8eeb749c13 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 b;\n" + " na::B b_c;\n" + " na::Two 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 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"