tom-anders created this revision.
tom-anders added a reviewer: sammccall.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
tom-anders updated this revision to Diff 466391.
tom-anders added a comment.
tom-anders published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
clang-format
Note that locateDeclAt() now does *not* canonicalize the declaration anymore,
since that would now lead to "AmbiguousSymbol" when there's a single
declaration under the cursor that has multiple canonical declarations.
Instead, only canonicalize *after* checking how many decls we've found under
the cursor.
This will allow renaming virtual methods with size_overridden_methods() > 1 in
a follow-up commit.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D135542
Files:
clang-tools-extra/clangd/refactor/Rename.cpp
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -82,35 +82,35 @@
//
// Here, both partial (2) and full (3) specializations are canonicalized to (1)
// which ensures all three of them are renamed.
-const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
+llvm::DenseSet<const NamedDecl *> canonicalRenameDecls(const NamedDecl *D) {
if (const auto *VarTemplate = dyn_cast<VarTemplateSpecializationDecl>(D))
- return canonicalRenameDecl(
+ return canonicalRenameDecls(
VarTemplate->getSpecializedTemplate()->getTemplatedDecl());
if (const auto *Template = dyn_cast<TemplateDecl>(D))
if (const NamedDecl *TemplatedDecl = Template->getTemplatedDecl())
- return canonicalRenameDecl(TemplatedDecl);
+ return canonicalRenameDecls(TemplatedDecl);
if (const auto *ClassTemplateSpecialization =
dyn_cast<ClassTemplateSpecializationDecl>(D))
- return canonicalRenameDecl(
+ return canonicalRenameDecls(
ClassTemplateSpecialization->getSpecializedTemplate()
->getTemplatedDecl());
if (const auto *Method = dyn_cast<CXXMethodDecl>(D)) {
if (Method->getDeclKind() == Decl::Kind::CXXConstructor ||
Method->getDeclKind() == Decl::Kind::CXXDestructor)
- return canonicalRenameDecl(Method->getParent());
+ return canonicalRenameDecls(Method->getParent());
if (const FunctionDecl *InstantiatedMethod =
Method->getInstantiatedFromMemberFunction())
- return canonicalRenameDecl(InstantiatedMethod);
+ return canonicalRenameDecls(InstantiatedMethod);
// FIXME(kirillbobyrev): For virtual methods with
// size_overridden_methods() > 1, this will not rename all functions it
// overrides, because this code assumes there is a single canonical
// declaration.
if (Method->isVirtual() && Method->size_overridden_methods())
- return canonicalRenameDecl(*Method->overridden_methods().begin());
+ return {canonicalRenameDecls(*Method->overridden_methods().begin())};
}
if (const auto *Function = dyn_cast<FunctionDecl>(D))
if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
- return canonicalRenameDecl(Template);
+ return canonicalRenameDecls(Template);
if (const auto *Field = dyn_cast<FieldDecl>(D)) {
// This is a hacky way to do something like
// CXXMethodDecl::getInstantiatedFromMemberFunction for the field because
@@ -119,25 +119,25 @@
const auto *FieldParent =
dyn_cast_or_null<CXXRecordDecl>(Field->getParent());
if (!FieldParent)
- return Field->getCanonicalDecl();
+ return {Field->getCanonicalDecl()};
FieldParent = FieldParent->getTemplateInstantiationPattern();
// Field is not instantiation.
if (!FieldParent || Field->getParent() == FieldParent)
- return Field->getCanonicalDecl();
+ return {Field->getCanonicalDecl()};
for (const FieldDecl *Candidate : FieldParent->fields())
if (Field->getDeclName() == Candidate->getDeclName())
- return Candidate->getCanonicalDecl();
+ return {Candidate->getCanonicalDecl()};
elog("FieldParent should have field with the same name as Field.");
}
if (const auto *VD = dyn_cast<VarDecl>(D)) {
if (const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember())
- return canonicalRenameDecl(OriginalVD);
+ return canonicalRenameDecls(OriginalVD);
}
if (const auto *UD = dyn_cast<UsingShadowDecl>(D)) {
if (const auto *TargetDecl = UD->getTargetDecl())
- return canonicalRenameDecl(TargetDecl);
+ return canonicalRenameDecls(TargetDecl);
}
- return dyn_cast<NamedDecl>(D->getCanonicalDecl());
+ return {dyn_cast<NamedDecl>(D->getCanonicalDecl())};
}
llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
@@ -156,7 +156,7 @@
targetDecl(SelectedNode->ASTNode,
DeclRelation::Alias | DeclRelation::TemplatePattern,
AST.getHeuristicResolver())) {
- Result.insert(canonicalRenameDecl(D));
+ Result.insert(D);
}
return Result;
}
@@ -267,7 +267,7 @@
std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
const NamedDecl &ND) {
trace::Span Tracer("FindOccurrencesWithinFile");
- assert(canonicalRenameDecl(&ND) == &ND &&
+ assert(canonicalRenameDecls(&ND).contains(&ND) &&
"ND should be already canonicalized.");
std::vector<SourceLocation> Results;
@@ -278,7 +278,7 @@
if (Ref.Targets.empty())
return;
for (const auto *Target : Ref.Targets) {
- if (canonicalRenameDecl(Target) == &ND) {
+ if (canonicalRenameDecls(Target).contains(&ND)) {
Results.push_back(Ref.NameLoc);
return;
}
@@ -529,34 +529,36 @@
// AST-based rename, it renames all occurrences in the main file.
llvm::Expected<tooling::Replacements>
-renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
+renameWithinFile(ParsedAST &AST,
+ const llvm::DenseSet<const NamedDecl *> &RenameDecls,
llvm::StringRef NewName) {
trace::Span Tracer("RenameWithinFile");
const SourceManager &SM = AST.getSourceManager();
tooling::Replacements FilteredChanges;
- for (SourceLocation Loc : findOccurrencesWithinFile(AST, RenameDecl)) {
- SourceLocation RenameLoc = Loc;
- // We don't rename in any macro bodies, but we allow rename the symbol
- // spelled in a top-level macro argument in the main file.
- if (RenameLoc.isMacroID()) {
- if (isInMacroBody(SM, RenameLoc))
+ for (const auto *RenameDecl : RenameDecls)
+ for (SourceLocation Loc : findOccurrencesWithinFile(AST, *RenameDecl)) {
+ SourceLocation RenameLoc = Loc;
+ // We don't rename in any macro bodies, but we allow rename the symbol
+ // spelled in a top-level macro argument in the main file.
+ if (RenameLoc.isMacroID()) {
+ if (isInMacroBody(SM, RenameLoc))
+ continue;
+ RenameLoc = SM.getSpellingLoc(Loc);
+ }
+ // Filter out locations not from main file.
+ // We traverse only main file decls, but locations could come from an
+ // non-preamble #include file e.g.
+ // void test() {
+ // int f^oo;
+ // #include "use_foo.inc"
+ // }
+ if (!isInsideMainFile(RenameLoc, SM))
continue;
- RenameLoc = SM.getSpellingLoc(Loc);
+ if (auto Err = FilteredChanges.add(tooling::Replacement(
+ SM, CharSourceRange::getTokenRange(RenameLoc), NewName)))
+ return std::move(Err);
}
- // Filter out locations not from main file.
- // We traverse only main file decls, but locations could come from an
- // non-preamble #include file e.g.
- // void test() {
- // int f^oo;
- // #include "use_foo.inc"
- // }
- if (!isInsideMainFile(RenameLoc, SM))
- continue;
- if (auto Err = FilteredChanges.add(tooling::Replacement(
- SM, CharSourceRange::getTokenRange(RenameLoc), NewName)))
- return std::move(Err);
- }
return FilteredChanges;
}
@@ -592,16 +594,20 @@
// Return all rename occurrences (using the index) outside of the main file,
// grouped by the absolute file path.
llvm::Expected<llvm::StringMap<std::vector<Range>>>
-findOccurrencesOutsideFile(const NamedDecl &RenameDecl,
+findOccurrencesOutsideFile(const llvm::DenseSet<const NamedDecl *> &RenameDecls,
llvm::StringRef MainFile, const SymbolIndex &Index,
size_t MaxLimitFiles) {
+ assert(!RenameDecls.empty() &&
+ "Should have at least one decl to rename here");
trace::Span Tracer("FindOccurrencesOutsideFile");
RefsRequest RQuest;
- RQuest.IDs.insert(getSymbolID(&RenameDecl));
- if (const auto *MethodDecl = llvm::dyn_cast<CXXMethodDecl>(&RenameDecl))
- if (MethodDecl->isVirtual())
- insertTransitiveOverrides(*RQuest.IDs.begin(), RQuest.IDs, Index);
+ for (const NamedDecl *D : RenameDecls) {
+ RQuest.IDs.insert(getSymbolID(D));
+ if (const auto *MethodDecl = llvm::dyn_cast<CXXMethodDecl>(D))
+ if (MethodDecl->isVirtual())
+ insertTransitiveOverrides(*RQuest.IDs.begin(), RQuest.IDs, Index);
+ }
// Absolute file path => rename occurrences in that file.
llvm::StringMap<std::vector<Range>> AffectedFiles;
@@ -621,7 +627,7 @@
MaxLimitFiles);
if (HasMore)
return error("The symbol {0} has too many occurrences",
- RenameDecl.getQualifiedNameAsString());
+ (*RenameDecls.begin())->getQualifiedNameAsString());
// Sort and deduplicate the results, in case that index returns duplications.
for (auto &FileAndOccurrences : AffectedFiles) {
auto &Ranges = FileAndOccurrences.getValue();
@@ -647,46 +653,47 @@
// as the file content we rename on, and fallback to file content on disk if
// there is no dirty buffer.
llvm::Expected<FileEdits>
-renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
- llvm::StringRef NewName, const SymbolIndex &Index,
- size_t MaxLimitFiles, llvm::vfs::FileSystem &FS) {
+renameOutsideFile(const llvm::DenseSet<const NamedDecl *> &RenameDecls,
+ llvm::StringRef MainFilePath, llvm::StringRef NewName,
+ const SymbolIndex &Index, size_t MaxLimitFiles,
+ llvm::vfs::FileSystem &FS) {
trace::Span Tracer("RenameOutsideFile");
- auto AffectedFiles = findOccurrencesOutsideFile(RenameDecl, MainFilePath,
+ auto AffectedFiles = findOccurrencesOutsideFile(RenameDecls, MainFilePath,
Index, MaxLimitFiles);
if (!AffectedFiles)
return AffectedFiles.takeError();
FileEdits Results;
- for (auto &FileAndOccurrences : *AffectedFiles) {
- llvm::StringRef FilePath = FileAndOccurrences.first();
-
- auto ExpBuffer = FS.getBufferForFile(FilePath);
- if (!ExpBuffer) {
- elog("Fail to read file content: Fail to open file {0}: {1}", FilePath,
- ExpBuffer.getError().message());
- continue;
- }
-
- auto AffectedFileCode = (*ExpBuffer)->getBuffer();
- auto RenameRanges =
- adjustRenameRanges(AffectedFileCode, RenameDecl.getNameAsString(),
- std::move(FileAndOccurrences.second),
- RenameDecl.getASTContext().getLangOpts());
- if (!RenameRanges) {
- // Our heuristics fails to adjust rename ranges to the current state of
- // the file, it is most likely the index is stale, so we give up the
- // entire rename.
- return error("Index results don't match the content of file {0} "
- "(the index may be stale)",
- FilePath);
+ for (const auto *RenameDecl : RenameDecls)
+ for (auto &FileAndOccurrences : *AffectedFiles) {
+ llvm::StringRef FilePath = FileAndOccurrences.first();
+
+ auto ExpBuffer = FS.getBufferForFile(FilePath);
+ if (!ExpBuffer) {
+ elog("Fail to read file content: Fail to open file {0}: {1}", FilePath,
+ ExpBuffer.getError().message());
+ continue;
+ }
+
+ auto AffectedFileCode = (*ExpBuffer)->getBuffer();
+ auto RenameRanges = adjustRenameRanges(
+ AffectedFileCode, RenameDecl->getNameAsString(),
+ FileAndOccurrences.second, RenameDecl->getASTContext().getLangOpts());
+ if (!RenameRanges) {
+ // Our heuristics fails to adjust rename ranges to the current state of
+ // the file, it is most likely the index is stale, so we give up the
+ // entire rename.
+ return error("Index results don't match the content of file {0} "
+ "(the index may be stale)",
+ FilePath);
+ }
+ auto RenameEdit =
+ buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewName);
+ if (!RenameEdit)
+ return error("failed to rename in file {0}: {1}", FilePath,
+ RenameEdit.takeError());
+ if (!RenameEdit->Replacements.empty())
+ Results.insert({FilePath, std::move(*RenameEdit)});
}
- auto RenameEdit =
- buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewName);
- if (!RenameEdit)
- return error("failed to rename in file {0}: {1}", FilePath,
- RenameEdit.takeError());
- if (!RenameEdit->Replacements.empty())
- Results.insert({FilePath, std::move(*RenameEdit)});
- }
return Results;
}
@@ -762,20 +769,28 @@
return makeError(ReasonToReject::NoSymbolFound);
if (DeclsUnderCursor.size() > 1)
return makeError(ReasonToReject::AmbiguousSymbol);
- const auto &RenameDecl = **DeclsUnderCursor.begin();
- const auto *ID = RenameDecl.getIdentifier();
- if (!ID)
- return makeError(ReasonToReject::UnsupportedSymbol);
- if (ID->getName() == RInputs.NewName)
- return makeError(ReasonToReject::SameName);
- auto Invalid = checkName(RenameDecl, RInputs.NewName);
- if (Invalid)
- return makeError(std::move(*Invalid));
- auto Reject =
- renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index, Opts);
- if (Reject)
- return makeError(*Reject);
+ llvm::DenseSet<const NamedDecl *> RenameDecls;
+ for (const auto *D : DeclsUnderCursor) {
+ auto CD = canonicalRenameDecls(D);
+ RenameDecls.insert(CD.begin(), CD.end());
+ }
+
+ for (const NamedDecl *D : RenameDecls) {
+ auto *ID = D->getIdentifier();
+ if (!ID)
+ return makeError(ReasonToReject::UnsupportedSymbol);
+ if (ID->getName() == RInputs.NewName)
+ return makeError(ReasonToReject::SameName);
+
+ auto Invalid = checkName(*D, RInputs.NewName);
+ if (Invalid)
+ return makeError(std::move(*Invalid));
+
+ auto Reject = renameable(*D, RInputs.MainFilePath, RInputs.Index, Opts);
+ if (Reject)
+ return makeError(*Reject);
+ }
// We have two implementations of the rename:
// - AST-based rename: used for renaming local symbols, e.g. variables
@@ -786,7 +801,7 @@
// To make cross-file rename work for local symbol, we use a hybrid solution:
// - run AST-based rename on the main file;
// - run index-based rename on other affected files;
- auto MainFileRenameEdit = renameWithinFile(AST, RenameDecl, RInputs.NewName);
+ auto MainFileRenameEdit = renameWithinFile(AST, RenameDecls, RInputs.NewName);
if (!MainFileRenameEdit)
return MainFileRenameEdit.takeError();
@@ -815,9 +830,12 @@
for (const TextEdit &TE : MainFileEdits.asTextEdits())
Result.LocalChanges.push_back(TE.range);
- // return the main file edit if this is a within-file rename or the symbol
- // being renamed is function local.
- if (RenameDecl.getParentFunctionOrMethod()) {
+ // return the main file edit if this is a within-file rename or the symbols
+ // being renamed are all function local.
+ auto IsWithinFileRename = [](const NamedDecl *D) {
+ return D->getParentFunctionOrMethod() != nullptr;
+ };
+ if (llvm::all_of(RenameDecls, IsWithinFileRename)) {
Result.GlobalChanges = FileEdits(
{std::make_pair(RInputs.MainFilePath, std::move(MainFileEdits))});
return Result;
@@ -831,7 +849,7 @@
}
auto OtherFilesEdits = renameOutsideFile(
- RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index,
+ RenameDecls, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index,
Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max()
: Opts.LimitFiles,
*RInputs.FS);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
