kbobyrev created this revision.
kbobyrev added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous,
MaskRay, ilya-biryukov.
Herald added a project: clang.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D74961
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/refactor/Rename.h
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/RenameTests.cpp
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -715,7 +715,8 @@
auto AST = TU.build();
llvm::StringRef NewName = "newName";
auto Results = rename({MainCode.point(), NewName, AST, MainFilePath,
- Index.get(), /*CrossFile=*/true, GetDirtyBuffer});
+ Index.get(), /*CrossFile=*/true,
+ /*GlobalRenameFileLimit=*/50, GetDirtyBuffer});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
applyEdits(std::move(*Results)),
@@ -731,7 +732,8 @@
TU.AdditionalFiles["bar.cc"] = std::string(BarCode.code());
AST = TU.build();
Results = rename({MainCode.point(), NewName, AST, MainFilePath, Index.get(),
- /*CrossFile=*/true, GetDirtyBuffer});
+ /*CrossFile=*/true, /*GlobalRenameFileLimit=*/50,
+ GetDirtyBuffer});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
applyEdits(std::move(*Results)),
@@ -762,7 +764,8 @@
size_t estimateMemoryUsage() const override { return 0; }
} PIndex;
Results = rename({MainCode.point(), NewName, AST, MainFilePath, &PIndex,
- /*CrossFile=*/true, GetDirtyBuffer});
+ /*CrossFile=*/true, /*GlobalRenameFileLimit=*/50,
+ GetDirtyBuffer});
EXPECT_FALSE(Results);
EXPECT_THAT(llvm::toString(Results.takeError()),
testing::HasSubstr("too many occurrences"));
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===================================================================
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -402,6 +402,14 @@
init(false),
};
+opt<unsigned> GlobalRenameFileLimit{
+ "global-rename-file-limit",
+ cat(Features),
+ desc("Renaming symbol that appears in more files will fail"),
+ init(50),
+ Hidden,
+};
+
/// Supports a test URI scheme with relaxed constraints for lit tests.
/// The path in a test URI will be combined with a platform-specific fake
/// directory to form an absolute path. For example, test:///a.cpp is resolved
@@ -601,6 +609,7 @@
}
ClangdServer::Options Opts;
+ Opts.GlobalRenameFileLimit = GlobalRenameFileLimit;
switch (PCHStorage) {
case PCHStorageFlag::Memory:
Opts.StorePreamblesInMemory = true;
Index: clang-tools-extra/clangd/refactor/Rename.h
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.h
+++ clang-tools-extra/clangd/refactor/Rename.h
@@ -36,6 +36,7 @@
const SymbolIndex *Index = nullptr;
bool AllowCrossFile = false;
+ const unsigned GlobalRenameFileLimit = 50;
// When set, used by the rename to get file content for all rename-related
// files.
// If there is no corresponding dirty buffer, we will use the file content
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -315,7 +315,8 @@
// grouped by the absolute file path.
llvm::Expected<llvm::StringMap<std::vector<Range>>>
findOccurrencesOutsideFile(const NamedDecl &RenameDecl,
- llvm::StringRef MainFile, const SymbolIndex &Index) {
+ llvm::StringRef MainFile, const SymbolIndex &Index,
+ const unsigned GlobalRenameFileLimit) {
trace::Span Tracer("FindOccurrencesOutsideFile");
RefsRequest RQuest;
RQuest.IDs.insert(*getSymbolID(&RenameDecl));
@@ -330,10 +331,8 @@
// Absolute file path => rename occurrences in that file.
llvm::StringMap<std::vector<Range>> AffectedFiles;
- // FIXME: Make the limit customizable.
- static constexpr size_t MaxLimitFiles = 50;
bool HasMore = Index.refs(RQuest, [&](const Ref &R) {
- if (AffectedFiles.size() > MaxLimitFiles)
+ if (AffectedFiles.size() > GlobalRenameFileLimit)
return;
if ((R.Kind & RefKind::Spelled) == RefKind::Unknown)
return;
@@ -343,10 +342,10 @@
}
});
- if (AffectedFiles.size() > MaxLimitFiles)
+ if (AffectedFiles.size() > GlobalRenameFileLimit)
return llvm::make_error<llvm::StringError>(
llvm::formatv("The number of affected files exceeds the max limit {0}",
- MaxLimitFiles),
+ GlobalRenameFileLimit),
llvm::inconvertibleErrorCode());
if (HasMore) {
return llvm::make_error<llvm::StringError>(
@@ -381,10 +380,11 @@
llvm::Expected<FileEdits> renameOutsideFile(
const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
llvm::StringRef NewName, const SymbolIndex &Index,
- llvm::function_ref<llvm::Expected<std::string>(PathRef)> GetFileContent) {
+ llvm::function_ref<llvm::Expected<std::string>(PathRef)> GetFileContent,
+ const unsigned GlobalRenameFileLimit) {
trace::Span Tracer("RenameOutsideFile");
- auto AffectedFiles =
- findOccurrencesOutsideFile(RenameDecl, MainFilePath, Index);
+ auto AffectedFiles = findOccurrencesOutsideFile(RenameDecl, MainFilePath,
+ Index, GlobalRenameFileLimit);
if (!AffectedFiles)
return AffectedFiles.takeError();
FileEdits Results;
@@ -541,9 +541,9 @@
// Renameable safely guards us that at this point we are renaming a local
// symbol if we don't have index.
if (RInputs.Index) {
- auto OtherFilesEdits =
- renameOutsideFile(RenameDecl, RInputs.MainFilePath, RInputs.NewName,
- *RInputs.Index, GetFileContent);
+ auto OtherFilesEdits = renameOutsideFile(
+ RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index,
+ GetFileContent, RInputs.GlobalRenameFileLimit);
if (!OtherFilesEdits)
return OtherFilesEdits.takeError();
Results = std::move(*OtherFilesEdits);
Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -145,6 +145,8 @@
/// Enable cross-file rename feature.
bool CrossFileRename = false;
+ unsigned GlobalRenameFileLimit = 50;
+
/// Returns true if the tweak should be enabled.
std::function<bool(const Tweak &)> TweakFilter = [](const Tweak &T) {
return !T.hidden(); // only enable non-hidden tweaks.
@@ -295,7 +297,7 @@
/// Get all document links in a file.
void documentLinks(PathRef File, Callback<std::vector<DocumentLink>> CB);
-
+
/// Returns estimated memory usage for each of the currently open files.
/// The order of results is unspecified.
/// Overall memory usage of clangd may be significantly more than reported
@@ -341,6 +343,7 @@
bool SuggestMissingIncludes = false;
bool CrossFileRename = false;
+ const unsigned GlobalRenameFileLimit = 50;
std::function<bool(const Tweak &)> TweakFilter;
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -131,8 +131,9 @@
: nullptr),
GetClangTidyOptions(Opts.GetClangTidyOptions),
SuggestMissingIncludes(Opts.SuggestMissingIncludes),
- CrossFileRename(Opts.CrossFileRename), TweakFilter(Opts.TweakFilter),
- WorkspaceRoot(Opts.WorkspaceRoot),
+ CrossFileRename(Opts.CrossFileRename),
+ GlobalRenameFileLimit(Opts.GlobalRenameFileLimit),
+ TweakFilter(Opts.TweakFilter), WorkspaceRoot(Opts.WorkspaceRoot),
// Pass a callback into `WorkScheduler` to extract symbols from a newly
// parsed file and rebuild the file index synchronously each time an AST
// is parsed.
@@ -344,9 +345,10 @@
// Performing the local rename isn't substantially more expensive than
// doing an AST-based check, so we just rename and throw away the results.
- auto Changes = clangd::rename({Pos, "dummy", AST, File, Index,
- /*AllowCrossFile=*/false,
- /*GetDirtyBuffer=*/nullptr});
+ auto Changes =
+ clangd::rename({Pos, "dummy", AST, File, Index,
+ /*AllowCrossFile=*/false, GlobalRenameFileLimit,
+ /*GetDirtyBuffer=*/nullptr});
if (!Changes) {
// LSP says to return null on failure, but that will result in a generic
// failure message. If we send an LSP error response, clients can surface
@@ -374,8 +376,9 @@
return llvm::None;
return It->second;
};
- auto Edits = clangd::rename({Pos, NewName, InpAST->AST, File, Index,
- CrossFileRename, GetDirtyBuffer});
+ auto Edits =
+ clangd::rename({Pos, NewName, InpAST->AST, File, Index, CrossFileRename,
+ GlobalRenameFileLimit, GetDirtyBuffer});
if (!Edits)
return CB(Edits.takeError());
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
