[clang-tools-extra] 7044f1d - [clangd] Use Dirty Filesystem for cross file rename.
Nathan James via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 10 05:41:38 PST 2021
Author: Nathan James
Date: 2021-03-10T13:41:29Z
New Revision: 7044f1d875e37a5badd4e59ee84b56faf7432f68
URL: https://github.com/llvm/llvm-project/commit/7044f1d875e37a5badd4e59ee84b56faf7432f68
DIFF: https://github.com/llvm/llvm-project/commit/7044f1d875e37a5badd4e59ee84b56faf7432f68.diff
LOG: [clangd] Use Dirty Filesystem for cross file rename.
Refactor cross file rename to use a Filesystem instead of a function for getting buffer contents of open files.
Depends on D94554
Reviewed By: sammccall
Differential Revision: https://reviews.llvm.org/D95043
Added:
Modified:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/refactor/Rename.h
clang-tools-extra/clangd/unittests/RenameTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 9a5f4bce2e21..df3dd363800a 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -490,9 +490,10 @@ void ClangdServer::prepareRename(PathRef File, Position Pos,
return CB(InpAST.takeError());
// prepareRename is latency-sensitive: we don't query the index, as we
// only need main-file references
- auto Results = clangd::rename(
- {Pos, NewName.getValueOr("__clangd_rename_dummy"), InpAST->AST, File,
- /*Index=*/nullptr, RenameOpts});
+ auto Results =
+ clangd::rename({Pos, NewName.getValueOr("__clangd_rename_dummy"),
+ InpAST->AST, File, /*FS=*/nullptr,
+ /*Index=*/nullptr, RenameOpts});
if (!Results) {
// 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
@@ -507,25 +508,16 @@ void ClangdServer::prepareRename(PathRef File, Position Pos,
void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
const RenameOptions &Opts,
Callback<RenameResult> CB) {
- // A snapshot of all file dirty buffers.
- llvm::StringMap<std::string> Snapshot = WorkScheduler->getAllFileContents();
auto Action = [File = File.str(), NewName = NewName.str(), Pos, Opts,
- CB = std::move(CB), Snapshot = std::move(Snapshot),
+ CB = std::move(CB),
this](llvm::Expected<InputsAndAST> InpAST) mutable {
// Tracks number of files edited per invocation.
static constexpr trace::Metric RenameFiles("rename_files",
trace::Metric::Distribution);
if (!InpAST)
return CB(InpAST.takeError());
- auto GetDirtyBuffer =
- [&Snapshot](PathRef AbsPath) -> llvm::Optional<std::string> {
- auto It = Snapshot.find(AbsPath);
- if (It == Snapshot.end())
- return llvm::None;
- return It->second;
- };
- auto R = clangd::rename(
- {Pos, NewName, InpAST->AST, File, Index, Opts, GetDirtyBuffer});
+ auto R = clangd::rename({Pos, NewName, InpAST->AST, File,
+ DirtyFS->view(llvm::None), Index, Opts});
if (!R)
return CB(R.takeError());
diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index 152aa5a49e8b..e76ef65922ee 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -394,7 +394,6 @@ class ClangdServer {
// Store of the current versions of the open documents.
// Only written from the main thread (despite being threadsafe).
- // FIXME: TUScheduler also keeps these, unify?
DraftStore DraftMgr;
std::unique_ptr<ThreadsafeFS> DirtyFS;
diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 306f690aac63..435deb6ec162 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -1519,13 +1519,6 @@ void TUScheduler::remove(PathRef File) {
// preamble of several open headers.
}
-llvm::StringMap<std::string> TUScheduler::getAllFileContents() const {
- llvm::StringMap<std::string> Results;
- for (auto &It : Files)
- Results.try_emplace(It.getKey(), It.getValue()->Contents);
- return Results;
-}
-
void TUScheduler::run(llvm::StringRef Name, llvm::StringRef Path,
llvm::unique_function<void()> Action) {
runWithSemaphore(Name, Path, std::move(Action), Barrier);
diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h
index 8749915aa757..8df2c019053e 100644
--- a/clang-tools-extra/clangd/TUScheduler.h
+++ b/clang-tools-extra/clangd/TUScheduler.h
@@ -235,9 +235,6 @@ class TUScheduler {
/// if requested with WantDiags::Auto or WantDiags::Yes.
void remove(PathRef File);
- /// Returns a snapshot of all file buffer contents, per last update().
- llvm::StringMap<std::string> getAllFileContents() const;
-
/// Schedule an async task with no dependencies.
/// Path may be empty (it is used only to set the Context).
void run(llvm::StringRef Name, llvm::StringRef Path,
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index cc6830d0ab37..853fc57bb906 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -586,10 +586,10 @@ findOccurrencesOutsideFile(const NamedDecl &RenameDecl,
// index (background index) is relatively stale. We choose the dirty buffers
// 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::function_ref<llvm::Expected<std::string>(PathRef)> GetFileContent) {
+llvm::Expected<FileEdits>
+renameOutsideFile(const NamedDecl &RenameDecl, 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,
Index, MaxLimitFiles);
@@ -599,13 +599,16 @@ llvm::Expected<FileEdits> renameOutsideFile(
for (auto &FileAndOccurrences : *AffectedFiles) {
llvm::StringRef FilePath = FileAndOccurrences.first();
- auto AffectedFileCode = GetFileContent(FilePath);
- if (!AffectedFileCode) {
- elog("Fail to read file content: {0}", AffectedFileCode.takeError());
+ 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(),
+ adjustRenameRanges(AffectedFileCode, RenameDecl.getNameAsString(),
std::move(FileAndOccurrences.second),
RenameDecl.getASTContext().getLangOpts());
if (!RenameRanges) {
@@ -617,7 +620,7 @@ llvm::Expected<FileEdits> renameOutsideFile(
FilePath);
}
auto RenameEdit =
- buildRenameEdit(FilePath, *AffectedFileCode, *RenameRanges, NewName);
+ buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewName);
if (!RenameEdit)
return error("failed to rename in file {0}: {1}", FilePath,
RenameEdit.takeError());
@@ -668,28 +671,13 @@ void findNearMiss(
} // namespace
llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
+ assert(!RInputs.Index == !RInputs.FS &&
+ "Index and FS must either both be specified or both null.");
trace::Span Tracer("Rename flow");
const auto &Opts = RInputs.Opts;
ParsedAST &AST = RInputs.AST;
const SourceManager &SM = AST.getSourceManager();
llvm::StringRef MainFileCode = SM.getBufferData(SM.getMainFileID());
- auto GetFileContent = [&RInputs,
- &SM](PathRef AbsPath) -> llvm::Expected<std::string> {
- llvm::Optional<std::string> DirtyBuffer;
- if (RInputs.GetDirtyBuffer &&
- (DirtyBuffer = RInputs.GetDirtyBuffer(AbsPath)))
- return std::move(*DirtyBuffer);
-
- auto Content =
- SM.getFileManager().getVirtualFileSystem().getBufferForFile(AbsPath);
- if (!Content)
- return error("Fail to open file {0}: {1}", AbsPath,
- Content.getError().message());
- if (!*Content)
- return error("Got no buffer for file {0}", AbsPath);
-
- return (*Content)->getBuffer().str();
- };
// Try to find the tokens adjacent to the cursor position.
auto Loc = sourceLocationInMainFile(SM, RInputs.Pos);
if (!Loc)
@@ -765,7 +753,7 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index,
Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max()
: Opts.LimitFiles,
- GetFileContent);
+ *RInputs.FS);
if (!OtherFilesEdits)
return OtherFilesEdits.takeError();
Result.GlobalChanges = *OtherFilesEdits;
diff --git a/clang-tools-extra/clangd/refactor/Rename.h b/clang-tools-extra/clangd/refactor/Rename.h
index 7eca4610eaca..03ae4f7f1563 100644
--- a/clang-tools-extra/clangd/refactor/Rename.h
+++ b/clang-tools-extra/clangd/refactor/Rename.h
@@ -21,11 +21,6 @@ namespace clangd {
class ParsedAST;
class SymbolIndex;
-/// Gets dirty buffer for a given file \p AbsPath.
-/// Returns None if there is no dirty buffer for the given file.
-using DirtyBufferGetter =
- llvm::function_ref<llvm::Optional<std::string>(PathRef AbsPath)>;
-
struct RenameOptions {
/// The maximum number of affected files (0 means no limit), only meaningful
/// when AllowCrossFile = true.
@@ -42,14 +37,14 @@ struct RenameInputs {
ParsedAST &AST;
llvm::StringRef MainFilePath;
+ // The filesystem to query when performing cross file renames.
+ // If this is set, Index must also be set, likewise if this is nullptr, Index
+ // must also be nullptr.
+ llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = nullptr;
+
const SymbolIndex *Index = nullptr;
RenameOptions Opts = {};
- // 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
- // from disk.
- DirtyBufferGetter GetDirtyBuffer = nullptr;
};
struct RenameResult {
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index 5e16640ccded..ca0e7ff24306 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -33,6 +33,19 @@ using testing::SizeIs;
using testing::UnorderedElementsAre;
using testing::UnorderedElementsAreArray;
+llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem>
+createOverlay(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> Base,
+ llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> Overlay) {
+ auto OFS =
+ llvm::makeIntrusiveRefCnt<llvm::vfs::OverlayFileSystem>(std::move(Base));
+ OFS->pushOverlay(std::move(Overlay));
+ return OFS;
+}
+
+llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> getVFSFromAST(ParsedAST &AST) {
+ return &AST.getSourceManager().getFileManager().getVirtualFileSystem();
+}
+
// Convert a Range to a Ref.
Ref refWithRange(const clangd::Range &Range, const std::string &URI) {
Ref Result;
@@ -815,7 +828,8 @@ TEST(RenameTest, WithinFileRename) {
auto Index = TU.index();
for (const auto &RenamePos : Code.points()) {
auto RenameResult =
- rename({RenamePos, NewName, AST, testPath(TU.Filename), Index.get()});
+ rename({RenamePos, NewName, AST, testPath(TU.Filename),
+ getVFSFromAST(AST), Index.get()});
ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
ASSERT_EQ(1u, RenameResult->GlobalChanges.size());
EXPECT_EQ(
@@ -1101,13 +1115,21 @@ TEST(RenameTest, IndexMergeMainFile) {
auto AST = TU.build();
auto Main = testPath("main.cc");
+ auto InMemFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+ InMemFS->addFile(testPath("main.cc"), 0,
+ llvm::MemoryBuffer::getMemBuffer(Code.code()));
+ InMemFS->addFile(testPath("other.cc"), 0,
+ llvm::MemoryBuffer::getMemBuffer(Code.code()));
auto Rename = [&](const SymbolIndex *Idx) {
- auto GetDirtyBuffer = [&](PathRef Path) -> llvm::Optional<std::string> {
- return Code.code().str(); // Every file has the same content.
- };
- RenameInputs Inputs{Code.point(), "xPrime", AST, Main,
- Idx, RenameOptions(), GetDirtyBuffer};
+ RenameInputs Inputs{Code.point(),
+ "xPrime",
+ AST,
+ Main,
+ Idx ? createOverlay(getVFSFromAST(AST), InMemFS)
+ : nullptr,
+ Idx,
+ RenameOptions()};
auto Results = rename(Inputs);
EXPECT_TRUE(bool(Results)) << llvm::toString(Results.takeError());
return std::move(*Results);
@@ -1237,25 +1259,19 @@ TEST(CrossFileRenameTests, DirtyBuffer) {
Annotations MainCode("class [[Fo^o]] {};");
auto MainFilePath = testPath("main.cc");
- // Dirty buffer for foo.cc.
- auto GetDirtyBuffer = [&](PathRef Path) -> llvm::Optional<std::string> {
- if (Path == FooPath)
- return FooDirtyBuffer.code().str();
- return llvm::None;
- };
+ llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemFS =
+ new llvm::vfs::InMemoryFileSystem;
+ InMemFS->addFile(FooPath, 0,
+ llvm::MemoryBuffer::getMemBuffer(FooDirtyBuffer.code()));
// Run rename on Foo, there is a dirty buffer for foo.cc, rename should
// respect the dirty buffer.
TestTU TU = TestTU::withCode(MainCode.code());
auto AST = TU.build();
llvm::StringRef NewName = "newName";
- auto Results = rename({MainCode.point(),
- NewName,
- AST,
- MainFilePath,
- Index.get(),
- {},
- GetDirtyBuffer});
+ auto Results =
+ rename({MainCode.point(), NewName, AST, MainFilePath,
+ createOverlay(getVFSFromAST(AST), InMemFS), Index.get()});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
applyEdits(std::move(Results->GlobalChanges)),
@@ -1270,13 +1286,8 @@ TEST(CrossFileRenameTests, DirtyBuffer) {
// Set a file "bar.cc" on disk.
TU.AdditionalFiles["bar.cc"] = std::string(BarCode.code());
AST = TU.build();
- Results = rename({MainCode.point(),
- NewName,
- AST,
- MainFilePath,
- Index.get(),
- {},
- GetDirtyBuffer});
+ Results = rename({MainCode.point(), NewName, AST, MainFilePath,
+ createOverlay(getVFSFromAST(AST), InMemFS), Index.get()});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
applyEdits(std::move(Results->GlobalChanges)),
@@ -1312,13 +1323,8 @@ TEST(CrossFileRenameTests, DirtyBuffer) {
size_t estimateMemoryUsage() const override { return 0; }
} PIndex;
- Results = rename({MainCode.point(),
- NewName,
- AST,
- MainFilePath,
- &PIndex,
- {},
- GetDirtyBuffer});
+ Results = rename({MainCode.point(), NewName, AST, MainFilePath,
+ createOverlay(getVFSFromAST(AST), InMemFS), &PIndex});
EXPECT_FALSE(Results);
EXPECT_THAT(llvm::toString(Results.takeError()),
testing::HasSubstr("too many occurrences"));
@@ -1368,8 +1374,8 @@ TEST(CrossFileRenameTests, DeduplicateRefsFromIndex) {
Ref ReturnedRef;
} DIndex(XRefInBarCC);
llvm::StringRef NewName = "newName";
- auto Results =
- rename({MainCode.point(), NewName, AST, MainFilePath, &DIndex});
+ auto Results = rename({MainCode.point(), NewName, AST, MainFilePath,
+ getVFSFromAST(AST), &DIndex});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
applyEdits(std::move(Results->GlobalChanges)),
More information about the cfe-commits
mailing list