ioeric created this revision.
ioeric added a reviewer: hokein.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
For example, a renamed type in a header file can conflict with declaration in
a random file that includes the header, but we should not consider the decl
ambiguous if
it's not visible at the rename location. This improves consistency of generated
replacements
when header file is included in different TUs.
Repository:
rC Clang
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: 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,36 @@
};
Visitor.runOver("namespace a { namespace b { class Foo {}; } }\n"
"namespace c { using a::b::Foo; Foo f();; }\n");
+
+ // Potentially ambiguous symbols that are not visible at reference location
+ // are not considered ambiguous.
+ Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+ if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
+ EXPECT_EQ("Foo", replaceRecordTypeLoc(Type, "::x::Foo"));
+ };
+ Visitor.runOver(R"(
+ // Definitions.
+ namespace x {
+ namespace y {
+ class Old {};
+ class Other {};
+ }
+ }
+
+ // Suppose this is a reference in some header file.
+ namespace x {
+ namespace y {
+ Old f; // This is to be renamed.
+ }
+ }
+
+ // Suppose this is in a file that includes the above header.
+ namespace x {
+ namespace y {
+ using Foo = ::x::y::Other;
+ }
+ }
+ )");
}
} // end anonymous namespace
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: 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
