njames93 created this revision.
njames93 added reviewers: sammccall, kadircet.
Herald added subscribers: usaxena95, arphaman, javed.absar.
njames93 requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.
Refactor cross file rename to use a Filesystem instead of a function for
getting buffer contents of open files.
Depends on D94554 <https://reviews.llvm.org/D94554>
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D95043
Files:
clang-tools-extra/clangd/ClangdServer.cpp
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;
@@ -833,8 +846,8 @@
TU.ExtraArgs.push_back("-xobjective-c++");
auto AST = TU.build();
for (const auto &RenamePos : Code.points()) {
- auto RenameResult =
- rename({RenamePos, NewName, AST, testPath(TU.Filename)});
+ auto RenameResult = rename(
+ {RenamePos, NewName, AST, testPath(TU.Filename), getVFSFromAST(AST)});
ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
ASSERT_EQ(1u, RenameResult->GlobalChanges.size());
EXPECT_EQ(
@@ -1040,8 +1053,8 @@
}
auto AST = TU.build();
llvm::StringRef NewName = Case.NewName;
- auto Results =
- rename({T.point(), NewName, AST, testPath(TU.Filename), Case.Index});
+ auto Results = rename({T.point(), NewName, AST, testPath(TU.Filename),
+ getVFSFromAST(AST), Case.Index});
bool WantRename = true;
if (T.ranges().empty())
WantRename = false;
@@ -1081,8 +1094,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,
@@ -1098,7 +1111,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"));
@@ -1170,12 +1184,10 @@
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.
@@ -1186,9 +1198,9 @@
NewName,
AST,
MainFilePath,
+ createOverlay(getVFSFromAST(AST), InMemFS),
Index.get(),
- {/*CrossFile=*/true},
- GetDirtyBuffer});
+ {/*CrossFile=*/true}});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
applyEdits(std::move(Results->GlobalChanges)),
@@ -1207,9 +1219,9 @@
NewName,
AST,
MainFilePath,
+ createOverlay(getVFSFromAST(AST), InMemFS),
Index.get(),
- {/*CrossFile=*/true},
- GetDirtyBuffer});
+ {/*CrossFile=*/true}});
ASSERT_TRUE(bool(Results)) << Results.takeError();
EXPECT_THAT(
applyEdits(std::move(Results->GlobalChanges)),
@@ -1249,9 +1261,9 @@
NewName,
AST,
MainFilePath,
+ createOverlay(getVFSFromAST(AST), InMemFS),
&PIndex,
- {/*CrossFile=*/true},
- GetDirtyBuffer});
+ {/*CrossFile=*/true}});
EXPECT_FALSE(Results);
EXPECT_THAT(llvm::toString(Results.takeError()),
testing::HasSubstr("too many occurrences"));
@@ -1305,6 +1317,7 @@
NewName,
AST,
MainFilePath,
+ getVFSFromAST(AST),
&DIndex,
{/*CrossFile=*/true}});
ASSERT_TRUE(bool(Results)) << Results.takeError();
@@ -1525,7 +1538,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 {
/// If true, enable cross-file rename; otherwise, only allows to rename a
/// symbol that's only used in the current file.
@@ -45,14 +40,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
@@ -509,10 +509,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);
@@ -522,13 +522,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) {
@@ -540,7 +543,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());
@@ -596,23 +599,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)
@@ -689,7 +675,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
@@ -239,9 +239,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
@@ -1330,13 +1330,6 @@
File);
}
-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) {
if (!PreambleTasks) {
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -197,8 +197,6 @@
}
if (DynamicIdx)
AddIndex(DynamicIdx.get());
- // Suppress warnings of this being unused, will be used later.
- (void)this->DirtyFS;
}
void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
@@ -372,6 +370,7 @@
// main-file occurrences;
auto Results = clangd::rename(
{Pos, NewName.getValueOr("__clangd_rename_dummy"), InpAST->AST, File,
+ (RenameOpts.AllowCrossFile ? DirtyFS : TFS).view(llvm::None),
RenameOpts.AllowCrossFile ? nullptr : Index, RenameOpts});
if (!Results) {
// LSP says to return null on failure, but that will result in a generic
@@ -387,25 +386,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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
