[PATCH] D93978: [clangd] Use dirty filesystem when performing cross file tweaks
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6b4e8f82a3f8: [clangd] Use dirty filesystem when performing
cross file tweaks (authored by njames93).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93978/new/
https://reviews.llvm.org/D93978
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
clang-tools-extra/clangd/tool/Check.cpp
clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp
clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
@@ -68,13 +68,14 @@
// Returns None if and only if prepare() failed.
llvm::Optional>
applyTweak(ParsedAST , const Annotations , StringRef TweakID,
- const SymbolIndex *Index) {
+ const SymbolIndex *Index, llvm::vfs::FileSystem *FS) {
auto Range = rangeOrPoint(Input);
llvm::Optional> Result;
SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), Range.first,
Range.second, [&](SelectionTree ST) {
Tweak::Selection S(Index, AST, Range.first,
- Range.second, std::move(ST));
+ Range.second, std::move(ST),
+ FS);
if (auto T = prepareTweak(TweakID, S, nullptr)) {
Result = (*T)->apply(S);
return true;
@@ -98,7 +99,9 @@
TU.ExtraArgs = ExtraArgs;
TU.AdditionalFiles = std::move(ExtraFiles);
ParsedAST AST = TU.build();
- auto Result = applyTweak(AST, Input, TweakID, Index);
+ auto Result = applyTweak(
+ AST, Input, TweakID, Index,
+ ().getFileManager().getVirtualFileSystem());
// We only care if prepare() succeeded, but must handle Errors.
if (Result && !*Result)
consumeError(Result->takeError());
@@ -119,7 +122,9 @@
TU.ExtraArgs = ExtraArgs;
ParsedAST AST = TU.build();
- auto Result = applyTweak(AST, Input, TweakID, Index.get());
+ auto Result = applyTweak(
+ AST, Input, TweakID, Index.get(),
+ ().getFileManager().getVirtualFileSystem());
if (!Result)
return "unavailable";
if (!*Result)
Index: clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp
===
--- clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp
+++ clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp
@@ -47,7 +47,8 @@
auto Tree =
SelectionTree::createRight(AST.getASTContext(), AST.getTokens(), 0, 0);
auto Actual = prepareTweak(
- TweakID, Tweak::Selection(nullptr, AST, 0, 0, std::move(Tree)), );
+ TweakID, Tweak::Selection(nullptr, AST, 0, 0, std::move(Tree), nullptr),
+ );
ASSERT_TRUE(bool(Actual));
EXPECT_EQ(Actual->get()->id(), TweakID);
}
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -210,7 +210,8 @@
vlog(" {0} {1}", Pos, Tok.text(AST->getSourceManager()));
auto Tree = SelectionTree::createRight(AST->getASTContext(),
AST->getTokens(), Start, End);
- Tweak::Selection Selection(, *AST, Start, End, std::move(Tree));
+ Tweak::Selection Selection(, *AST, Start, End, std::move(Tree),
+ nullptr);
for (const auto :
prepareTweaks(Selection, Opts.TweakFilter, Opts.FeatureModules)) {
auto Result = T->apply(Selection);
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -64,9 +64,8 @@
llvm::Optional getSourceFile(llvm::StringRef FileName,
const Tweak::Selection ) {
- if (auto Source = getCorrespondingHeaderOrSource(
- FileName,
- >getSourceManager().getFileManager().getVirtualFileSystem()))
+ assert(Sel.FS);
+ if (auto Source = getCorrespondingHeaderOrSource(FileName, Sel.FS))
return *Source;
return getCorrespondingHeaderOrSource(FileName, *Sel.AST, Sel.Index);
}
@@ -414,12 +413,11 @@
return error("Couldn't get absolute path for main file.");
auto CCFile = getSourceFile(*MainFileName, Sel);
+
if (!CCFile)
return
[PATCH] D93978: [clangd] Use dirty filesystem when performing cross file tweaks
njames93 updated this revision to Diff 338518.
njames93 added a comment.
Fix up compilation failures and test failures.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93978/new/
https://reviews.llvm.org/D93978
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
clang-tools-extra/clangd/tool/Check.cpp
clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp
clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
@@ -68,13 +68,14 @@
// Returns None if and only if prepare() failed.
llvm::Optional>
applyTweak(ParsedAST , const Annotations , StringRef TweakID,
- const SymbolIndex *Index) {
+ const SymbolIndex *Index, llvm::vfs::FileSystem *FS) {
auto Range = rangeOrPoint(Input);
llvm::Optional> Result;
SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), Range.first,
Range.second, [&](SelectionTree ST) {
Tweak::Selection S(Index, AST, Range.first,
- Range.second, std::move(ST));
+ Range.second, std::move(ST),
+ FS);
if (auto T = prepareTweak(TweakID, S, nullptr)) {
Result = (*T)->apply(S);
return true;
@@ -98,7 +99,9 @@
TU.ExtraArgs = ExtraArgs;
TU.AdditionalFiles = std::move(ExtraFiles);
ParsedAST AST = TU.build();
- auto Result = applyTweak(AST, Input, TweakID, Index);
+ auto Result = applyTweak(
+ AST, Input, TweakID, Index,
+ ().getFileManager().getVirtualFileSystem());
// We only care if prepare() succeeded, but must handle Errors.
if (Result && !*Result)
consumeError(Result->takeError());
@@ -119,7 +122,9 @@
TU.ExtraArgs = ExtraArgs;
ParsedAST AST = TU.build();
- auto Result = applyTweak(AST, Input, TweakID, Index.get());
+ auto Result = applyTweak(
+ AST, Input, TweakID, Index.get(),
+ ().getFileManager().getVirtualFileSystem());
if (!Result)
return "unavailable";
if (!*Result)
Index: clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp
===
--- clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp
+++ clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp
@@ -47,7 +47,8 @@
auto Tree =
SelectionTree::createRight(AST.getASTContext(), AST.getTokens(), 0, 0);
auto Actual = prepareTweak(
- TweakID, Tweak::Selection(nullptr, AST, 0, 0, std::move(Tree)), );
+ TweakID, Tweak::Selection(nullptr, AST, 0, 0, std::move(Tree), nullptr),
+ );
ASSERT_TRUE(bool(Actual));
EXPECT_EQ(Actual->get()->id(), TweakID);
}
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -210,7 +210,8 @@
vlog(" {0} {1}", Pos, Tok.text(AST->getSourceManager()));
auto Tree = SelectionTree::createRight(AST->getASTContext(),
AST->getTokens(), Start, End);
- Tweak::Selection Selection(, *AST, Start, End, std::move(Tree));
+ Tweak::Selection Selection(, *AST, Start, End, std::move(Tree),
+ nullptr);
for (const auto :
prepareTweaks(Selection, Opts.TweakFilter, Opts.FeatureModules)) {
auto Result = T->apply(Selection);
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -64,9 +64,8 @@
llvm::Optional getSourceFile(llvm::StringRef FileName,
const Tweak::Selection ) {
- if (auto Source = getCorrespondingHeaderOrSource(
- FileName,
- >getSourceManager().getFileManager().getVirtualFileSystem()))
+ assert(Sel.FS);
+ if (auto Source = getCorrespondingHeaderOrSource(FileName, Sel.FS))
return *Source;
return getCorrespondingHeaderOrSource(FileName, *Sel.AST, Sel.Index);
}
@@ -414,12 +413,11 @@
return error("Couldn't get absolute path for main file.");
auto CCFile = getSourceFile(*MainFileName, Sel);
+
if (!CCFile)
return error("Couldn't find a suitable implementation file.");
-
-auto =
-
[PATCH] D93978: [clangd] Use dirty filesystem when performing cross file tweaks
njames93 updated this revision to Diff 338294.
njames93 marked 4 inline comments as done.
njames93 added a comment.
Address comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93978/new/
https://reviews.llvm.org/D93978
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
clang-tools-extra/clangd/tool/Check.cpp
clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
@@ -74,7 +74,8 @@
SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), Range.first,
Range.second, [&](SelectionTree ST) {
Tweak::Selection S(Index, AST, Range.first,
- Range.second, std::move(ST));
+ Range.second, std::move(ST),
+ nullptr);
if (auto T = prepareTweak(TweakID, S, nullptr)) {
Result = (*T)->apply(S);
return true;
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -210,7 +210,8 @@
vlog(" {0} {1}", Pos, Tok.text(AST->getSourceManager()));
auto Tree = SelectionTree::createRight(AST->getASTContext(),
AST->getTokens(), Start, End);
- Tweak::Selection Selection(, *AST, Start, End, std::move(Tree));
+ Tweak::Selection Selection(, *AST, Start, End, std::move(Tree),
+ nullptr);
for (const auto :
prepareTweaks(Selection, Opts.TweakFilter, Opts.FeatureModules)) {
auto Result = T->apply(Selection);
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -64,9 +64,8 @@
llvm::Optional getSourceFile(llvm::StringRef FileName,
const Tweak::Selection ) {
- if (auto Source = getCorrespondingHeaderOrSource(
- FileName,
- >getSourceManager().getFileManager().getVirtualFileSystem()))
+ assert(Sel.FS);
+ if (auto Source = getCorrespondingHeaderOrSource(FileName, Sel.FS))
return *Source;
return getCorrespondingHeaderOrSource(FileName, *Sel.AST, Sel.Index);
}
@@ -414,12 +413,11 @@
return error("Couldn't get absolute path for main file.");
auto CCFile = getSourceFile(*MainFileName, Sel);
+
if (!CCFile)
return error("Couldn't find a suitable implementation file.");
-
-auto =
-Sel.AST->getSourceManager().getFileManager().getVirtualFileSystem();
-auto Buffer = FS.getBufferForFile(*CCFile);
+assert(Sel.FS && "FS Must be set in apply");
+auto Buffer = Sel.FS->getBufferForFile(*CCFile);
// FIXME: Maybe we should consider creating the implementation file if it
// doesn't exist?
if (!Buffer)
Index: clang-tools-extra/clangd/refactor/Tweak.h
===
--- clang-tools-extra/clangd/refactor/Tweak.h
+++ clang-tools-extra/clangd/refactor/Tweak.h
@@ -51,7 +51,8 @@
/// Input to prepare and apply tweaks.
struct Selection {
Selection(const SymbolIndex *Index, ParsedAST , unsigned RangeBegin,
- unsigned RangeEnd, SelectionTree ASTSelection);
+ unsigned RangeEnd, SelectionTree ASTSelection,
+ llvm::vfs::FileSystem *VFS);
/// The text of the active document.
llvm::StringRef Code;
/// The Index for handling codebase related queries.
@@ -67,6 +68,9 @@
unsigned SelectionEnd;
/// The AST nodes that were selected.
SelectionTree ASTSelection;
+/// File system used to access source code (for cross-file tweaks).
+/// This is only populated when applying a tweak, not during prepare.
+llvm::vfs::FileSystem *FS = nullptr;
// FIXME: provide a way to get sources and ASTs for other files.
};
Index: clang-tools-extra/clangd/refactor/Tweak.cpp
===
--- clang-tools-extra/clangd/refactor/Tweak.cpp
+++ clang-tools-extra/clangd/refactor/Tweak.cpp
@@ -61,9 +61,10 @@
Tweak::Selection::Selection(const SymbolIndex *Index, ParsedAST ,
unsigned RangeBegin,
[PATCH] D93978: [clangd] Use dirty filesystem when performing cross file tweaks
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Herald added a project: clang-tools-extra.
Sorry, I thought this had already gone in!
LG, sorry for the back and forth.
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:575
return CB(InpAST.takeError());
-auto Selections = tweakSelection(Sel, *InpAST);
+// FIXME: Should we use the dirty fs here?
+auto FS = TFS.view(llvm::None);
njames93 wrote:
> Regarding this, Is it wise to set the contract so that Tweak::prepare isn't
> allowed to do IO so we should just pass a nullptr here, inline with how
> prepareRename works.
Yes, I think so.
Comment at: clang-tools-extra/clangd/refactor/Tweak.cpp:54
+ SelectionEnd(RangeEnd), ASTSelection(std::move(ASTSelection)),
+ FS(FS ? FS
+: ().getFileManager().getVirtualFileSystem())
{
I'm no longer convinced this fallback is a good idea.
We want prepare() to do no IO, ideally we'd assert if we do. But by falling
back to the AST FS, we'll mask that problem.
There are only two callsites that need this fallback (TweakTesting & Check) - I
think we should just inline what we mean there.
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:67
+ const Tweak::Selection ,
+ llvm::vfs::FileSystem *FS) {
+ if (auto Source = getCorrespondingHeaderOrSource(FileName, FS))
why take this as an explicit parameter rather than just accessing (and
asserting) Sel.FS?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93978/new/
https://reviews.llvm.org/D93978
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93978: [clangd] Use dirty filesystem when performing cross file tweaks
njames93 added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:575 return CB(InpAST.takeError()); -auto Selections = tweakSelection(Sel, *InpAST); +// FIXME: Should we use the dirty fs here? +auto FS = TFS.view(llvm::None); Regarding this, Is it wise to set the contract so that Tweak::prepare isn't allowed to do IO so we should just pass a nullptr here, inline with how prepareRename works. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93978/new/ https://reviews.llvm.org/D93978 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93978: [clangd] Use dirty filesystem when performing cross file tweaks
njames93 updated this revision to Diff 329649.
njames93 added a comment.
Rebased.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93978/new/
https://reviews.llvm.org/D93978
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
clang-tools-extra/clangd/tool/Check.cpp
clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp
@@ -74,7 +74,8 @@
SelectionTree::createEach(AST.getASTContext(), AST.getTokens(), Range.first,
Range.second, [&](SelectionTree ST) {
Tweak::Selection S(Index, AST, Range.first,
- Range.second, std::move(ST));
+ Range.second, std::move(ST),
+ nullptr);
if (auto T = prepareTweak(TweakID, S)) {
Result = (*T)->apply(S);
return true;
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -203,7 +203,8 @@
vlog(" {0} {1}", Pos, Tok.text(AST->getSourceManager()));
auto Tree = SelectionTree::createRight(AST->getASTContext(),
AST->getTokens(), Start, End);
- Tweak::Selection Selection(, *AST, Start, End, std::move(Tree));
+ Tweak::Selection Selection(, *AST, Start, End, std::move(Tree),
+ nullptr);
for (const auto : prepareTweaks(Selection, Opts.TweakFilter)) {
auto Result = T->apply(Selection);
if (!Result) {
Index: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -63,10 +63,9 @@
}
llvm::Optional getSourceFile(llvm::StringRef FileName,
- const Tweak::Selection ) {
- if (auto Source = getCorrespondingHeaderOrSource(
- FileName,
- >getSourceManager().getFileManager().getVirtualFileSystem()))
+ const Tweak::Selection ,
+ llvm::vfs::FileSystem *FS) {
+ if (auto Source = getCorrespondingHeaderOrSource(FileName, FS))
return *Source;
return getCorrespondingHeaderOrSource(FileName, *Sel.AST, Sel.Index);
}
@@ -409,13 +408,14 @@
if (!MainFileName)
return error("Couldn't get absolute path for main file.");
-auto CCFile = getSourceFile(*MainFileName, Sel);
+auto *FS = Sel.FS;
+assert(FS && "FS Must be set in apply");
+
+auto CCFile = getSourceFile(*MainFileName, Sel, FS);
+
if (!CCFile)
return error("Couldn't find a suitable implementation file.");
-
-auto =
-Sel.AST->getSourceManager().getFileManager().getVirtualFileSystem();
-auto Buffer = FS.getBufferForFile(*CCFile);
+auto Buffer = FS->getBufferForFile(*CCFile);
// FIXME: Maybe we should consider creating the implementation file if it
// doesn't exist?
if (!Buffer)
Index: clang-tools-extra/clangd/refactor/Tweak.h
===
--- clang-tools-extra/clangd/refactor/Tweak.h
+++ clang-tools-extra/clangd/refactor/Tweak.h
@@ -48,7 +48,8 @@
/// Input to prepare and apply tweaks.
struct Selection {
Selection(const SymbolIndex *Index, ParsedAST , unsigned RangeBegin,
- unsigned RangeEnd, SelectionTree ASTSelection);
+ unsigned RangeEnd, SelectionTree ASTSelection,
+ llvm::vfs::FileSystem *VFS);
/// The text of the active document.
llvm::StringRef Code;
/// The Index for handling codebase related queries.
@@ -64,6 +65,11 @@
unsigned SelectionEnd;
/// The AST nodes that were selected.
SelectionTree ASTSelection;
+/// File system used to access source code (for cross-file tweaks).
+/// This can be used to overlay the "dirty" contents of files open in the
+/// editor, which (in case of headers) may not match the saved contents used
+/// for building the AST.
+llvm::vfs::FileSystem *FS = nullptr;
// FIXME: provide a way to get sources and ASTs for other files.
};
Index: clang-tools-extra/clangd/refactor/Tweak.cpp
