[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
This commit is contained in:
Eric Liu 2017-03-21 12:41:59 +00:00
parent 3b939d070c
commit 8bc2416414
2 changed files with 124 additions and 62 deletions

View File

@ -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);

View File

@ -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"