This revision was automatically updated to reflect the committed changes. Closed by commit rL344897: [change-namespace] Enhance detection of conflicting namespaces. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits.
Repository: rL LLVM https://reviews.llvm.org/D53489 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 @@ -2244,6 +2244,39 @@ EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code)); } +TEST_F(ChangeNamespaceTest, FullyQualifyConflictNamespace) { + std::string Code = + "namespace x { namespace util { class Some {}; } }\n" + "namespace x { namespace y {namespace base { class Base {}; } } }\n" + "namespace util { class Status {}; }\n" + "namespace base { class Base {}; }\n" + "namespace na {\n" + "namespace nb {\n" + "void f() {\n" + " util::Status s1; x::util::Some s2;\n" + " base::Base b1; x::y::base::Base b2;\n" + "}\n" + "} // namespace nb\n" + "} // namespace na\n"; + + std::string Expected = + "namespace x { namespace util { class Some {}; } }\n" + "namespace x { namespace y {namespace base { class Base {}; } } }\n" + "namespace util { class Status {}; }\n" + "namespace base { class Base {}; }\n" + "\n" + "namespace x {\n" + "namespace y {\n" + "void f() {\n" + " ::util::Status s1; util::Some s2;\n" + " ::base::Base b1; base::Base b2;\n" + "}\n" + "} // namespace y\n" + "} // namespace x\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 @@ -7,8 +7,10 @@ // //===----------------------------------------------------------------------===// #include "ChangeNamespace.h" +#include "clang/AST/ASTContext.h" #include "clang/Format/Format.h" #include "clang/Lex/Lexer.h" +#include "llvm/Support/Casting.h" #include "llvm/Support/ErrorHandling.h" using namespace clang::ast_matchers; @@ -283,23 +285,48 @@ } // Given a qualified symbol name, returns true if the symbol will be -// incorrectly qualified without leading "::". -bool conflictInNamespace(llvm::StringRef QualifiedSymbol, +// incorrectly qualified without leading "::". For example, a symbol +// "nx::ny::Foo" in namespace "na::nx::ny" without leading "::"; a symbol +// "util::X" in namespace "na" can potentially conflict with "na::util" (if this +// exists). +bool conflictInNamespace(const ASTContext &AST, 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()) { + if (SymbolSplitted.size() >= 1 && !Namespace.empty()) { + auto SymbolTopNs = SymbolSplitted.front(); 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. + + auto LookupDecl = [&AST](const Decl &Scope, + llvm::StringRef Name) -> const NamedDecl * { + const auto *DC = llvm::dyn_cast<DeclContext>(&Scope); + if (!DC) + return nullptr; + auto LookupRes = DC->lookup(DeclarationName(&AST.Idents.get(Name))); + if (LookupRes.empty()) + return nullptr; + return LookupRes.front(); + }; + // 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. + const NamedDecl *Scope = + LookupDecl(*AST.getTranslationUnitDecl(), NsSplitted.front()); for (auto I = NsSplitted.begin() + 1, E = NsSplitted.end(); I != E; ++I) { - if (*I == SymbolSplitted.front()) + if (*I == SymbolTopNs) // Handles "::ny" in "::nx::ny" case. return true; + // Handles "::util" and "::nx::util" conflicts. + if (Scope) { + if (LookupDecl(*Scope, SymbolTopNs)) + return true; + Scope = LookupDecl(*Scope, *I); + } } + if (Scope && LookupDecl(*Scope, SymbolTopNs)) + return true; } return false; } @@ -844,15 +871,16 @@ } } } + bool Conflict = conflictInNamespace(DeclCtx->getParentASTContext(), + ReplaceName, NewNamespace); // If the new nested name in the new namespace is the same as it was in the - // old namespace, we don't create replacement. - if (NestedName == ReplaceName || + // old namespace, we don't create replacement unless there can be ambiguity. + if ((NestedName == ReplaceName && !Conflict) || (NestedName.startswith("::") && NestedName.drop_front(2) == ReplaceName)) return; // If the reference need to be fully-qualified, add a leading "::" unless // NewNamespace is the global namespace. - if (ReplaceName == FromDeclName && !NewNamespace.empty() && - conflictInNamespace(ReplaceName, NewNamespace)) + if (ReplaceName == FromDeclName && !NewNamespace.empty() && Conflict) ReplaceName = "::" + ReplaceName; addReplacementOrDie(Start, End, ReplaceName, *Result.SourceManager, &FileToReplacements);
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits