njames93 updated this revision to Diff 329126.
njames93 added a comment.
Rebase and remove an addressed FIXME.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95043/new/
https://reviews.llvm.org/D95043
Files:
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
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -33,6 +33,19 @@
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 @@
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(
@@ -1072,7 +1086,8 @@
}
auto AST = TU.build();
llvm::StringRef NewName = Case.NewName;
- auto Results = rename({T.point(), NewName, AST, testPath(TU.Filename)});
+ auto Results = rename(
+ {T.point(), NewName, AST, testPath(TU.Filename), getVFSFromAST(AST)});
bool WantRename = true;
if (T.ranges().empty())
WantRename = false;
@@ -1101,13 +1116,20 @@
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,
+ createOverlay(getVFSFromAST(AST), InMemFS),
+ Idx,
+ RenameOptions()};
auto Results = rename(Inputs);
EXPECT_TRUE(bool(Results)) << llvm::toString(Results.takeError());
return std::move(*Results);
@@ -1156,8 +1178,8 @@
auto AST = TU.build();
llvm::StringRef NewName = "abcde";
- auto RenameResult =
- rename({Code.point(), NewName, AST, testPath(TU.Filename)});
+ auto RenameResult = rename(
+ {Code.point(), NewName, AST, testPath(TU.Filename), getVFSFromAST(AST)});
ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError() << Code.point();
ASSERT_EQ(1u, RenameResult->GlobalChanges.size());
EXPECT_EQ(applyEdits(std::move(RenameResult->GlobalChanges)).front().second,
@@ -1173,7 +1195,8 @@
)cpp";
TU.HeaderFilename = "protobuf.pb.h";
auto AST = TU.build();
- auto Results = rename({Code.point(), "newName", AST, testPath(TU.Filename)});
+ auto Results = rename({Code.point(), "newName", AST, testPath(TU.Filename),
+ getVFSFromAST(AST)});
EXPECT_FALSE(Results);
EXPECT_THAT(llvm::toString(Results.takeError()),
testing::HasSubstr("not a supported kind"));
@@ -1237,25 +1260,19 @@
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 +1287,8 @@
// 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 +1324,8 @@
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 +1375,8 @@
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)),
@@ -1588,7 +1595,7 @@
auto Path = testPath(TU.Filename);
auto AST = TU.build();
llvm::StringRef NewName = "newName";
- auto Results = rename({Code.point(), NewName, AST, Path});
+ auto Results = rename({Code.point(), NewName, AST, Path, getVFSFromAST(AST)});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
applyEdits(std::move(Results->GlobalChanges)),
Index: clang-tools-extra/clangd/refactor/Rename.h
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.h
+++ clang-tools-extra/clangd/refactor/Rename.h
@@ -21,11 +21,6 @@
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,12 @@
ParsedAST &AST;
llvm::StringRef MainFilePath;
+ // The filesystem to query when performing cross file renames.
+ llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS;
+
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 {
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -586,10 +586,10 @@
// 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 @@
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 @@
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());
@@ -673,23 +676,6 @@
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 +751,7 @@
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;
Index: clang-tools-extra/clangd/TUScheduler.h
===================================================================
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -235,9 +235,6 @@
/// 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,
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -1519,13 +1519,6 @@
// 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);
Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -392,7 +392,6 @@
// 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;
const std::unique_ptr<ThreadsafeFS> DirtyFS;
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -484,15 +484,16 @@
const RenameOptions &RenameOpts,
Callback<RenameResult> CB) {
auto Action = [Pos, File = File.str(), CB = std::move(CB),
- NewName = std::move(NewName),
+ NewName = std::move(NewName), this,
RenameOpts](llvm::Expected<InputsAndAST> InpAST) mutable {
if (!InpAST)
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, TFS.view(llvm::None),
+ /*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::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());
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
