Author: ioeric Date: Tue Mar 19 03:12:15 2019 New Revision: 356446 URL: http://llvm.org/viewvc/llvm-project?rev=356446&view=rev Log: [Tooling] Add more scope specifiers until spelling is not ambiguous.
Summary: Previously, when the renamed spelling is ambiguous, we simply use the full-qualfied name (with leading "::"). This patch makes it try adding additional specifiers one at a time until name is no longer ambiguous, which allows us to find better disambuguated spelling. Reviewers: kadircet, gribozavr Subscribers: jdoerfert, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D59487 Modified: cfe/trunk/lib/Tooling/Core/Lookup.cpp cfe/trunk/unittests/Tooling/LookupTest.cpp Modified: cfe/trunk/lib/Tooling/Core/Lookup.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Core/Lookup.cpp?rev=356446&r1=356445&r2=356446&view=diff ============================================================================== --- cfe/trunk/lib/Tooling/Core/Lookup.cpp (original) +++ cfe/trunk/lib/Tooling/Core/Lookup.cpp Tue Mar 19 03:12:15 2019 @@ -14,6 +14,7 @@ #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/DeclarationName.h" +#include "llvm/ADT/SmallVector.h" using namespace clang; using namespace clang::tooling; @@ -114,35 +115,60 @@ static bool isFullyQualified(const Neste return false; } -// Returns true if spelling symbol \p QName as \p Spelling in \p UseContext is -// ambiguous. For example, if QName is "::y::bar" and the spelling is "y::bar" -// in `UseContext` "a" that contains a nested namespace "a::y", then "y::bar" -// can be resolved to ::a::y::bar, which can cause compile error. +// Adds more scope specifier to the spelled name until the spelling is not +// ambiguous. A spelling is ambiguous if the resolution of the symbol is +// ambiguous. For example, if QName is "::y::bar", the spelling is "y::bar", and +// context contains a nested namespace "a::y", then "y::bar" can be resolved to +// ::a::y::bar in the context, which can cause compile error. // FIXME: consider using namespaces. -static bool isAmbiguousNameInScope(StringRef Spelling, StringRef QName, - const DeclContext &UseContext) { +static std::string disambiguateSpellingInScope(StringRef Spelling, + StringRef QName, + const DeclContext &UseContext) { assert(QName.startswith("::")); + assert(QName.endswith(Spelling)); if (Spelling.startswith("::")) - return false; + return Spelling; - // Lookup the first component of Spelling in all enclosing namespaces and - // check if there is any existing symbols with the same name but in different - // scope. - StringRef Head = Spelling.split("::").first; + auto UnspelledSpecifier = QName.drop_back(Spelling.size()); + llvm::SmallVector<llvm::StringRef, 2> UnspelledScopes; + UnspelledSpecifier.split(UnspelledScopes, "::", /*MaxSplit=*/-1, + /*KeepEmpty=*/false); - llvm::SmallVector<const NamespaceDecl *, 4> UseNamespaces = + llvm::SmallVector<const NamespaceDecl *, 4> EnclosingNamespaces = getAllNamedNamespaces(&UseContext); auto &AST = UseContext.getParentASTContext(); StringRef TrimmedQName = QName.substr(2); - for (const auto *NS : UseNamespaces) { - auto LookupRes = NS->lookup(DeclarationName(&AST.Idents.get(Head))); - if (!LookupRes.empty()) { - for (const NamedDecl *Res : LookupRes) - if (!TrimmedQName.startswith(Res->getQualifiedNameAsString())) - return true; + + auto IsAmbiguousSpelling = [&EnclosingNamespaces, &AST, &TrimmedQName]( + const llvm::StringRef CurSpelling) { + if (CurSpelling.startswith("::")) + return false; + // Lookup the first component of Spelling in all enclosing namespaces + // and check if there is any existing symbols with the same name but in + // different scope. + StringRef Head = CurSpelling.split("::").first; + for (const auto *NS : EnclosingNamespaces) { + auto LookupRes = NS->lookup(DeclarationName(&AST.Idents.get(Head))); + if (!LookupRes.empty()) { + for (const NamedDecl *Res : LookupRes) + if (!TrimmedQName.startswith(Res->getQualifiedNameAsString())) + return true; + } + } + return false; + }; + + // Add more qualifiers until the spelling is not ambiguous. + std::string Disambiguated = Spelling; + while (IsAmbiguousSpelling(Disambiguated)) { + if (UnspelledScopes.empty()) { + Disambiguated = "::" + Disambiguated; + } else { + Disambiguated = (UnspelledScopes.back() + "::" + Disambiguated).str(); + UnspelledScopes.pop_back(); } } - return false; + return Disambiguated; } std::string tooling::replaceNestedName(const NestedNameSpecifier *Use, @@ -179,12 +205,6 @@ std::string tooling::replaceNestedName(c // specific). StringRef Suggested = getBestNamespaceSubstr(UseContext, ReplacementString, isFullyQualified(Use)); - // Use the fully qualified name if the suggested name is ambiguous. - // FIXME: consider re-shortening the name until the name is not ambiguous. We - // are not doing this because ambiguity is pretty bad and we should not try to - // be clever in handling such cases. Making this noticeable to users seems to - // be a better option. - return isAmbiguousNameInScope(Suggested, ReplacementString, *UseContext) - ? ReplacementString - : Suggested; + + return disambiguateSpellingInScope(Suggested, ReplacementString, *UseContext); } Modified: cfe/trunk/unittests/Tooling/LookupTest.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/LookupTest.cpp?rev=356446&r1=356445&r2=356446&view=diff ============================================================================== --- cfe/trunk/unittests/Tooling/LookupTest.cpp (original) +++ cfe/trunk/unittests/Tooling/LookupTest.cpp Tue Mar 19 03:12:15 2019 @@ -129,20 +129,38 @@ TEST(LookupTest, replaceNestedFunctionNa // If the shortest name is ambiguous, we need to add more qualifiers. Visitor.OnCall = [&](CallExpr *Expr) { - EXPECT_EQ("::a::y::bar", replaceCallExpr(Expr, "::a::y::bar")); + EXPECT_EQ("a::y::bar", replaceCallExpr(Expr, "::a::y::bar")); }; Visitor.runOver(R"( namespace a { - namespace b { - namespace x { void foo() {} } - namespace y { void foo() {} } - } + namespace b { + namespace x { void foo() {} } + namespace y { void foo() {} } + } } namespace a { - namespace b { - void f() { x::foo(); } + namespace b { + void f() { x::foo(); } + } + })"); + + Visitor.OnCall = [&](CallExpr *Expr) { + // y::bar would be ambiguous due to "a::b::y". + EXPECT_EQ("::y::bar", replaceCallExpr(Expr, "::y::bar")); + }; + Visitor.runOver(R"( + namespace a { + namespace b { + void foo() {} + namespace y { } + } } + + namespace a { + namespace b { + void f() { foo(); } + } })"); Visitor.OnCall = [&](CallExpr *Expr) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits