Skip to content

Commit

Permalink
[clang-rename] Don't add prefix qualifiers to the declaration and def…
Browse files Browse the repository at this point in the history 
…inition of the renamed symbol.

Reviewers: ioeric

Reviewed By: ioeric

Subscribers: klimek, cfe-commits, arphaman

Differential Revision: https://reviews.llvm.org/D38723

llvm-svn: 315452
  • Loading branch information
@hokein
hokein committed Oct 11, 2017
1 parent 41851e3 commit 19d7299
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 40 deletions.
75 changes: 49 additions & 26 deletions clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
View file
Expand Up @@ -160,13 +160,14 @@ class RenameLocFinder : public RecursiveASTVisitor<RenameLocFinder> {
const Decl *Context;
// The nested name being replaced (can be nullptr).
const NestedNameSpecifier *Specifier;
// Determine whether the prefix qualifiers of the NewName should be ignored.
// Normally, we set it to true for the symbol declaration and definition to
// avoid adding prefix qualifiers.
// For example, if it is true and NewName is "a::b::foo", then the symbol
// occurrence which the RenameInfo points to will be renamed to "foo".
bool IgnorePrefixQualifers;
};

// FIXME: Currently, prefix qualifiers will be added to the renamed symbol
// definition (e.g. "class Foo {};" => "class b::Bar {};" when renaming
// "a::Foo" to "b::Bar").
// For renaming declarations/definitions, prefix qualifiers should be filtered
// out.
bool VisitNamedDecl(const NamedDecl *Decl) {
// UsingDecl has been handled in other place.
if (llvm::isa<UsingDecl>(Decl))
Expand All @@ -180,8 +181,12 @@ class RenameLocFinder : public RecursiveASTVisitor<RenameLocFinder> {
return true;

if (isInUSRSet(Decl)) {
RenameInfo Info = {Decl->getLocation(), Decl->getLocation(), nullptr,
nullptr, nullptr};
RenameInfo Info = {Decl->getLocation(),
Decl->getLocation(),
/*FromDecl=*/nullptr,
/*Context=*/nullptr,
/*Specifier=*/nullptr,
/*IgnorePrefixQualifers=*/true};
RenameInfos.push_back(Info);
}
return true;
Expand All @@ -191,8 +196,11 @@ class RenameLocFinder : public RecursiveASTVisitor<RenameLocFinder> {
const NamedDecl *Decl = Expr->getFoundDecl();
if (isInUSRSet(Decl)) {
RenameInfo Info = {Expr->getSourceRange().getBegin(),
Expr->getSourceRange().getEnd(), Decl,
getClosestAncestorDecl(*Expr), Expr->getQualifier()};
Expr->getSourceRange().getEnd(),
Decl,
getClosestAncestorDecl(*Expr),
Expr->getQualifier(),
/*IgnorePrefixQualifers=*/false};
RenameInfos.push_back(Info);
}

Expand Down Expand Up @@ -220,8 +228,10 @@ class RenameLocFinder : public RecursiveASTVisitor<RenameLocFinder> {
if (isInUSRSet(TargetDecl)) {
RenameInfo Info = {NestedLoc.getBeginLoc(),
EndLocationForType(NestedLoc.getTypeLoc()),
TargetDecl, getClosestAncestorDecl(NestedLoc),
NestedLoc.getNestedNameSpecifier()->getPrefix()};
TargetDecl,
getClosestAncestorDecl(NestedLoc),
NestedLoc.getNestedNameSpecifier()->getPrefix(),
/*IgnorePrefixQualifers=*/false};
RenameInfos.push_back(Info);
}
}
Expand Down Expand Up @@ -265,9 +275,12 @@ class RenameLocFinder : public RecursiveASTVisitor<RenameLocFinder> {
if (!ParentTypeLoc.isNull() &&
isInUSRSet(getSupportedDeclFromTypeLoc(ParentTypeLoc)))
return true;
RenameInfo Info = {StartLocationForType(Loc), EndLocationForType(Loc),
TargetDecl, getClosestAncestorDecl(Loc),
GetNestedNameForType(Loc)};
RenameInfo Info = {StartLocationForType(Loc),
EndLocationForType(Loc),
TargetDecl,
getClosestAncestorDecl(Loc),
GetNestedNameForType(Loc),
/*IgnorePrefixQualifers=*/false};
RenameInfos.push_back(Info);
return true;
}
Expand All @@ -293,11 +306,13 @@ class RenameLocFinder : public RecursiveASTVisitor<RenameLocFinder> {
llvm::isa<ElaboratedType>(ParentTypeLoc.getType()))
TargetLoc = ParentTypeLoc;
RenameInfo Info = {
StartLocationForType(TargetLoc), EndLocationForType(TargetLoc),
StartLocationForType(TargetLoc),
EndLocationForType(TargetLoc),
TemplateSpecType->getTemplateName().getAsTemplateDecl(),
getClosestAncestorDecl(
ast_type_traits::DynTypedNode::create(TargetLoc)),
GetNestedNameForType(TargetLoc)};
GetNestedNameForType(TargetLoc),
/*IgnorePrefixQualifers=*/false};
RenameInfos.push_back(Info);
}
}
Expand Down Expand Up @@ -423,18 +438,26 @@ createRenameAtomicChanges(llvm::ArrayRef<std::string> USRs,

for (const auto &RenameInfo : Finder.getRenameInfos()) {
std::string ReplacedName = NewName.str();
if (RenameInfo.FromDecl && RenameInfo.Context) {
if (!llvm::isa<clang::TranslationUnitDecl>(
RenameInfo.Context->getDeclContext())) {
ReplacedName = tooling::replaceNestedName(
RenameInfo.Specifier, RenameInfo.Context->getDeclContext(),
RenameInfo.FromDecl,
NewName.startswith("::") ? NewName.str() : ("::" + NewName).str());
if (RenameInfo.IgnorePrefixQualifers) {
// Get the name without prefix qualifiers from NewName.
size_t LastColonPos = NewName.find_last_of(':');
if (LastColonPos != std::string::npos)
ReplacedName = NewName.substr(LastColonPos + 1);
} else {
if (RenameInfo.FromDecl && RenameInfo.Context) {
if (!llvm::isa<clang::TranslationUnitDecl>(
RenameInfo.Context->getDeclContext())) {
ReplacedName = tooling::replaceNestedName(
RenameInfo.Specifier, RenameInfo.Context->getDeclContext(),
RenameInfo.FromDecl,
NewName.startswith("::") ? NewName.str()
: ("::" + NewName).str());
}
}
// If the NewName contains leading "::", add it back.
if (NewName.startswith("::") && NewName.substr(2) == ReplacedName)
ReplacedName = NewName.str();
}
// If the NewName contains leading "::", add it back.
if (NewName.startswith("::") && NewName.substr(2) == ReplacedName)
ReplacedName = NewName.str();
Replace(RenameInfo.Begin, RenameInfo.End, ReplacedName);
}

Expand Down
24 changes: 10 additions & 14 deletions clang/unittests/Rename/RenameClassTest.cpp
View file
Expand Up @@ -469,8 +469,6 @@ TEST_F(ClangRenameTest, RenameClassWithInlineMembers) {
CompareSnippets(Expected, After);
}

// FIXME: no prefix qualifiers being added to the class definition and
// constructor.
TEST_F(ClangRenameTest, RenameClassWithNamespaceWithInlineMembers) {
std::string Before = R"(
namespace ns {
Expand All @@ -488,9 +486,9 @@ TEST_F(ClangRenameTest, RenameClassWithNamespaceWithInlineMembers) {
)";
std::string Expected = R"(
namespace ns {
class ns::New {
class New {
public:
ns::New() {}
New() {}
~New() {}
New* next() { return next_; }
Expand All @@ -504,8 +502,6 @@ TEST_F(ClangRenameTest, RenameClassWithNamespaceWithInlineMembers) {
CompareSnippets(Expected, After);
}

// FIXME: no prefix qualifiers being added to the class definition and
// constructor.
TEST_F(ClangRenameTest, RenameClassWithNamespaceWithOutOfInlineMembers) {
std::string Before = R"(
namespace ns {
Expand All @@ -527,9 +523,9 @@ TEST_F(ClangRenameTest, RenameClassWithNamespaceWithOutOfInlineMembers) {
)";
std::string Expected = R"(
namespace ns {
class ns::New {
class New {
public:
ns::New();
New();
~New();
New* next();
Expand All @@ -538,7 +534,7 @@ TEST_F(ClangRenameTest, RenameClassWithNamespaceWithOutOfInlineMembers) {
New* next_;
};
New::ns::New() {}
New::New() {}
New::~New() {}
New* New::next() { return next_; }
} // namespace ns
Expand All @@ -547,12 +543,12 @@ TEST_F(ClangRenameTest, RenameClassWithNamespaceWithOutOfInlineMembers) {
CompareSnippets(Expected, After);
}

// FIXME: no prefix qualifiers being added to the definition.
TEST_F(ClangRenameTest, RenameClassInInheritedConstructor) {
// `using Base::Base;` will generate an implicit constructor containing usage
// of `::ns::Old` which should not be matched.
std::string Before = R"(
namespace ns {
class Old;
class Old {
int x;
};
Expand All @@ -574,7 +570,8 @@ TEST_F(ClangRenameTest, RenameClassInInheritedConstructor) {
})";
std::string Expected = R"(
namespace ns {
class ns::New {
class New;
class New {
int x;
};
class Base {
Expand Down Expand Up @@ -615,7 +612,7 @@ TEST_F(ClangRenameTest, DontRenameReferencesInImplicitFunction) {
)";
std::string Expected = R"(
namespace ns {
class ::new_ns::New {
class New {
};
} // namespace ns
struct S {
Expand All @@ -632,7 +629,6 @@ TEST_F(ClangRenameTest, DontRenameReferencesInImplicitFunction) {
CompareSnippets(Expected, After);
}

// FIXME: no prefix qualifiers being adding to the definition.
TEST_F(ClangRenameTest, ReferencesInLambdaFunctionParameters) {
std::string Before = R"(
template <class T>
Expand Down Expand Up @@ -669,7 +665,7 @@ TEST_F(ClangRenameTest, ReferencesInLambdaFunctionParameters) {
};
namespace ns {
class ::new_ns::New {};
class New {};
void f() {
function<void(::new_ns::New)> func;
}
Expand Down

0 comments on commit 19d7299

Please sign in to comment.