[PATCH] D93978: [clangd] Use dirty filesystem when performing cross file tweaks

[Sam McCall via Phabricator via cfe-commits]

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
+            : &AST.getSourceManager().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 &Sel,
+                                   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