r338832 - Fully qualify the renamed symbol if the shortened name is ambiguous.
Eric Liu via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 3 02:16:07 PDT 2018
Author: ioeric
Date: Fri Aug 3 02:16:07 2018
New Revision: 338832
URL: http://llvm.org/viewvc/llvm-project?rev=338832&view=rev
Log:
Fully qualify the renamed symbol if the shortened name is ambiguous.
Summary:
For example, when renaming `a::b::x::foo` to `y::foo` below, replacing
`x::foo()` with `y::foo()` can cause ambiguity. In such cases, we simply fully
qualify the name with leading `::`.
```
namespace a {
namespace b {
namespace x { void foo() {} }
namespace y { void foo() {} }
}
}
namespace a {
namespace b {
void f() { x::foo(); }
}
}
```
Reviewers: ilya-biryukov, hokein
Reviewed By: ilya-biryukov
Subscribers: cfe-commits
Differential Revision: https://reviews.llvm.org/D50189
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=338832&r1=338831&r2=338832&view=diff
==============================================================================
--- cfe/trunk/lib/Tooling/Core/Lookup.cpp (original)
+++ cfe/trunk/lib/Tooling/Core/Lookup.cpp Fri Aug 3 02:16:07 2018
@@ -14,6 +14,7 @@
#include "clang/Tooling/Core/Lookup.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclarationName.h"
using namespace clang;
using namespace clang::tooling;
@@ -114,6 +115,37 @@ 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.
+// FIXME: consider using namespaces.
+static bool isAmbiguousNameInScope(StringRef Spelling, StringRef QName,
+ const DeclContext &UseContext) {
+ assert(QName.startswith("::"));
+ if (Spelling.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 = Spelling.split("::").first;
+
+ llvm::SmallVector<const NamespaceDecl *, 4> UseNamespaces =
+ 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;
+ }
+ }
+ return false;
+}
+
std::string tooling::replaceNestedName(const NestedNameSpecifier *Use,
const DeclContext *UseContext,
const NamedDecl *FromDecl,
@@ -146,6 +178,14 @@ std::string tooling::replaceNestedName(c
// figure out how good a namespace match we have with our destination type.
// We work backwards (from most specific possible namespace to least
// specific).
- return getBestNamespaceSubstr(UseContext, ReplacementString,
- isFullyQualified(Use));
+ 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;
}
Modified: cfe/trunk/unittests/Tooling/LookupTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/LookupTest.cpp?rev=338832&r1=338831&r2=338832&view=diff
==============================================================================
--- cfe/trunk/unittests/Tooling/LookupTest.cpp (original)
+++ cfe/trunk/unittests/Tooling/LookupTest.cpp Fri Aug 3 02:16:07 2018
@@ -68,7 +68,7 @@ TEST(LookupTest, replaceNestedFunctionNa
"namespace b { void f() { a::foo(); } }\n");
Visitor.OnCall = [&](CallExpr *Expr) {
- EXPECT_EQ("a::bar", replaceCallExpr(Expr, "::a::bar"));
+ EXPECT_EQ("::a::bar", replaceCallExpr(Expr, "::a::bar"));
};
Visitor.runOver("namespace a { void foo(); }\n"
"namespace b { namespace a { void foo(); }\n"
@@ -127,6 +127,38 @@ TEST(LookupTest, replaceNestedFunctionNa
"namespace a { namespace b { namespace c {"
"void f() { foo(); }"
"} } }\n");
+
+ // 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"));
+ };
+ Visitor.runOver(R"(
+ namespace a {
+ namespace b {
+ namespace x { void foo() {} }
+ namespace y { void foo() {} }
+ }
+ }
+
+ namespace a {
+ namespace b {
+ void f() { x::foo(); }
+ }
+ })");
+
+ Visitor.OnCall = [&](CallExpr *Expr) {
+ EXPECT_EQ("y::bar", replaceCallExpr(Expr, "::y::bar"));
+ };
+ Visitor.runOver(R"(
+ namespace a {
+ namespace b {
+ namespace x { void foo() {} }
+ namespace y { void foo() {} }
+ }
+ }
+
+ void f() { a::b::x::foo(); }
+ )");
}
TEST(LookupTest, replaceNestedClassName) {
More information about the cfe-commits
mailing list