[clang-tools-extra] r298363 - [change-namespace] avoid adding leading '::' when possible.
Eric Liu via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 21 05:42:00 PDT 2017
Author: ioeric
Date: Tue Mar 21 07:41:59 2017
New Revision: 298363
URL: http://llvm.org/viewvc/llvm-project?rev=298363&view=rev
Log:
[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
Modified:
clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
Modified: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp?rev=298363&r1=298362&r2=298363&view=diff
==============================================================================
--- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp (original)
+++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp Tue Mar 21 07:41:59 2017
@@ -28,6 +28,14 @@ joinNamespaces(const llvm::SmallVectorIm
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(c
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 getShortestQualifiedNameInNa
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 getShortestQualifiedNameInNa
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 Sourc
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::replaceQualifi
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);
Modified: clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp?rev=298363&r1=298362&r2=298363&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp Tue Mar 21 07:41:59 2017
@@ -242,7 +242,7 @@ TEST_F(ChangeNamespaceTest, MoveIntoExis
"} // 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, SimpleMoveWi
"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, TypeLocInTem
"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, LeaveForward
"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, InsertForwar
"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, TemplateClas
"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, MoveFunction
"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, FixUsingShad
"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, DontFixUsing
"} // 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, TypeInNested
"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, FixFunctionN
"} // 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, FixOverloade
"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, FixNonCallin
"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, MoveAndFixGl
"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, DoNotFixStat
"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, NamespaceAli
"}\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, NamespaceAli
"}\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, UsingDeclInM
"} // 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, UsingDeclInM
"} // 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, DerivedClass
" 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, KeepGlobalSp
"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, ExistingName
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, ReferencesTo
"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, NoRedundantE
" 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, TypeAsTempla
" TempTemp(t);\n"
"}\n"
"void f() {\n"
- " ::na::X x;\n"
+ " na::X x;\n"
" Temp(x);\n"
"}\n"
"} // namespace y\n"
More information about the cfe-commits
mailing list