This revision was automatically updated to reflect the committed changes.
Closed by commit rC358378: [Lookup] Invisible decls should not be ambiguous
when renaming. (authored by ioeric, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D60257?vs=194841&id=195103#toc
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60257/new/
https://reviews.llvm.org/D60257
Files:
include/clang/Tooling/Core/Lookup.h
lib/Tooling/Core/Lookup.cpp
lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
unittests/Tooling/LookupTest.cpp
Index: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===================================================================
--- lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -542,8 +542,8 @@
if (!llvm::isa<clang::TranslationUnitDecl>(
RenameInfo.Context->getDeclContext())) {
ReplacedName = tooling::replaceNestedName(
- RenameInfo.Specifier, RenameInfo.Context->getDeclContext(),
- RenameInfo.FromDecl,
+ RenameInfo.Specifier, RenameInfo.Begin,
+ RenameInfo.Context->getDeclContext(), RenameInfo.FromDecl,
NewName.startswith("::") ? NewName.str()
: ("::" + NewName).str());
} else {
Index: lib/Tooling/Core/Lookup.cpp
===================================================================
--- lib/Tooling/Core/Lookup.cpp
+++ lib/Tooling/Core/Lookup.cpp
@@ -14,6 +14,7 @@
#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclarationName.h"
+#include "clang/Basic/SourceLocation.h"
#include "llvm/ADT/SmallVector.h"
using namespace clang;
using namespace clang::tooling;
@@ -123,7 +124,8 @@
// FIXME: consider using namespaces.
static std::string disambiguateSpellingInScope(StringRef Spelling,
StringRef QName,
- const DeclContext &UseContext) {
+ const DeclContext &UseContext,
+ SourceLocation UseLoc) {
assert(QName.startswith("::"));
assert(QName.endswith(Spelling));
if (Spelling.startswith("::"))
@@ -138,9 +140,10 @@
getAllNamedNamespaces(&UseContext);
auto &AST = UseContext.getParentASTContext();
StringRef TrimmedQName = QName.substr(2);
+ const auto &SM = UseContext.getParentASTContext().getSourceManager();
+ UseLoc = SM.getSpellingLoc(UseLoc);
- auto IsAmbiguousSpelling = [&EnclosingNamespaces, &AST, &TrimmedQName](
- const llvm::StringRef CurSpelling) {
+ auto IsAmbiguousSpelling = [&](const llvm::StringRef CurSpelling) {
if (CurSpelling.startswith("::"))
return false;
// Lookup the first component of Spelling in all enclosing namespaces
@@ -151,7 +154,13 @@
auto LookupRes = NS->lookup(DeclarationName(&AST.Idents.get(Head)));
if (!LookupRes.empty()) {
for (const NamedDecl *Res : LookupRes)
- if (!TrimmedQName.startswith(Res->getQualifiedNameAsString()))
+ // If `Res` is not visible in `UseLoc`, we don't consider it
+ // ambiguous. For example, a reference in a header file should not be
+ // affected by a potentially ambiguous name in some file that includes
+ // the header.
+ if (!TrimmedQName.startswith(Res->getQualifiedNameAsString()) &&
+ SM.isBeforeInTranslationUnit(
+ SM.getSpellingLoc(Res->getLocation()), UseLoc))
return true;
}
}
@@ -172,6 +181,7 @@
}
std::string tooling::replaceNestedName(const NestedNameSpecifier *Use,
+ SourceLocation UseLoc,
const DeclContext *UseContext,
const NamedDecl *FromDecl,
StringRef ReplacementString) {
@@ -206,5 +216,6 @@
StringRef Suggested = getBestNamespaceSubstr(UseContext, ReplacementString,
isFullyQualified(Use));
- return disambiguateSpellingInScope(Suggested, ReplacementString, *UseContext);
+ return disambiguateSpellingInScope(Suggested, ReplacementString, *UseContext,
+ UseLoc);
}
Index: unittests/Tooling/LookupTest.cpp
===================================================================
--- unittests/Tooling/LookupTest.cpp
+++ unittests/Tooling/LookupTest.cpp
@@ -44,8 +44,8 @@
const auto *Callee = cast<DeclRefExpr>(Expr->getCallee()->IgnoreImplicit());
const ValueDecl *FD = Callee->getDecl();
return tooling::replaceNestedName(
- Callee->getQualifier(), Visitor.DeclStack.back()->getDeclContext(), FD,
- ReplacementString);
+ Callee->getQualifier(), Callee->getLocation(),
+ Visitor.DeclStack.back()->getDeclContext(), FD, ReplacementString);
};
Visitor.OnCall = [&](CallExpr *Expr) {
@@ -181,12 +181,12 @@
TEST(LookupTest, replaceNestedClassName) {
GetDeclsVisitor Visitor;
- auto replaceRecordTypeLoc = [&](RecordTypeLoc Loc,
+ auto replaceRecordTypeLoc = [&](RecordTypeLoc TLoc,
StringRef ReplacementString) {
- const auto *FD = cast<CXXRecordDecl>(Loc.getDecl());
+ const auto *FD = cast<CXXRecordDecl>(TLoc.getDecl());
return tooling::replaceNestedName(
- nullptr, Visitor.DeclStack.back()->getDeclContext(), FD,
- ReplacementString);
+ nullptr, TLoc.getBeginLoc(), Visitor.DeclStack.back()->getDeclContext(),
+ FD, ReplacementString);
};
Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
@@ -211,6 +211,40 @@
};
Visitor.runOver("namespace a { namespace b { class Foo {}; } }\n"
"namespace c { using a::b::Foo; Foo f();; }\n");
+
+ // Rename TypeLoc `x::y::Old` to new name `x::Foo` at [0] and check that the
+ // type is replaced with "Foo" instead of "x::Foo". Although there is a symbol
+ // `x::y::Foo` in c.cc [1], it should not make "Foo" at [0] ambiguous because
+ // it's not visible at [0].
+ Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+ if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
+ EXPECT_EQ("Foo", replaceRecordTypeLoc(Type, "::x::Foo"));
+ };
+ Visitor.runOver(R"(
+ // a.h
+ namespace x {
+ namespace y {
+ class Old {};
+ class Other {};
+ }
+ }
+
+ // b.h
+ namespace x {
+ namespace y {
+ // This is to be renamed to x::Foo
+ // The expected replacement is "Foo".
+ Old f; // [0].
+ }
+ }
+
+ // c.cc
+ namespace x {
+ namespace y {
+ using Foo = ::x::y::Other; // [1]
+ }
+ }
+ )");
}
} // end anonymous namespace
Index: include/clang/Tooling/Core/Lookup.h
===================================================================
--- include/clang/Tooling/Core/Lookup.h
+++ include/clang/Tooling/Core/Lookup.h
@@ -14,6 +14,7 @@
#define LLVM_CLANG_TOOLING_CORE_LOOKUP_H
#include "clang/Basic/LLVM.h"
+#include "clang/Basic/SourceLocation.h"
#include <string>
namespace clang {
@@ -30,6 +31,7 @@
/// This does not perform a full C++ lookup so ADL will not work.
///
/// \param Use The nested name to be replaced.
+/// \param UseLoc The location of name to be replaced.
/// \param UseContext The context in which the nested name is contained. This
/// will be used to minimize namespace qualifications.
/// \param FromDecl The declaration to which the nested name points.
@@ -37,6 +39,7 @@
/// qualified including a leading "::".
/// \returns The new name to be inserted in place of the current nested name.
std::string replaceNestedName(const NestedNameSpecifier *Use,
+ SourceLocation UseLoc,
const DeclContext *UseContext,
const NamedDecl *FromDecl,
StringRef ReplacementString);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
