r358378 - [Lookup] Invisible decls should not be ambiguous when renaming.
Eric Liu via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 15 01:46:34 PDT 2019
Author: ioeric
Date: Mon Apr 15 01:46:34 2019
New Revision: 358378
URL: http://llvm.org/viewvc/llvm-project?rev=358378&view=rev
Log:
[Lookup] Invisible decls should not be ambiguous when renaming.
Summary:
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.
Reviewers: hokein
Subscribers: cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D60257
Modified:
cfe/trunk/include/clang/Tooling/Core/Lookup.h
cfe/trunk/lib/Tooling/Core/Lookup.cpp
cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
cfe/trunk/unittests/Tooling/LookupTest.cpp
Modified: cfe/trunk/include/clang/Tooling/Core/Lookup.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Core/Lookup.h?rev=358378&r1=358377&r2=358378&view=diff
==============================================================================
--- cfe/trunk/include/clang/Tooling/Core/Lookup.h (original)
+++ cfe/trunk/include/clang/Tooling/Core/Lookup.h Mon Apr 15 01:46:34 2019
@@ -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 @@ namespace tooling {
/// 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 @@ namespace tooling {
/// 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);
Modified: cfe/trunk/lib/Tooling/Core/Lookup.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Core/Lookup.cpp?rev=358378&r1=358377&r2=358378&view=diff
==============================================================================
--- cfe/trunk/lib/Tooling/Core/Lookup.cpp (original)
+++ cfe/trunk/lib/Tooling/Core/Lookup.cpp Mon Apr 15 01:46:34 2019
@@ -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 @@ static bool isFullyQualified(const Neste
// 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 @@ static std::string disambiguateSpellingI
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 @@ static std::string disambiguateSpellingI
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 @@ static std::string disambiguateSpellingI
}
std::string tooling::replaceNestedName(const NestedNameSpecifier *Use,
+ SourceLocation UseLoc,
const DeclContext *UseContext,
const NamedDecl *FromDecl,
StringRef ReplacementString) {
@@ -206,5 +216,6 @@ std::string tooling::replaceNestedName(c
StringRef Suggested = getBestNamespaceSubstr(UseContext, ReplacementString,
isFullyQualified(Use));
- return disambiguateSpellingInScope(Suggested, ReplacementString, *UseContext);
+ return disambiguateSpellingInScope(Suggested, ReplacementString, *UseContext,
+ UseLoc);
}
Modified: cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp?rev=358378&r1=358377&r2=358378&view=diff
==============================================================================
--- cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp (original)
+++ cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp Mon Apr 15 01:46:34 2019
@@ -542,8 +542,8 @@ createRenameAtomicChanges(llvm::ArrayRef
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 {
Modified: cfe/trunk/unittests/Tooling/LookupTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/LookupTest.cpp?rev=358378&r1=358377&r2=358378&view=diff
==============================================================================
--- cfe/trunk/unittests/Tooling/LookupTest.cpp (original)
+++ cfe/trunk/unittests/Tooling/LookupTest.cpp Mon Apr 15 01:46:34 2019
@@ -44,8 +44,8 @@ TEST(LookupTest, replaceNestedFunctionNa
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, replaceNestedFunctionNa
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 @@ TEST(LookupTest, replaceNestedClassName)
};
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
More information about the cfe-commits
mailing list