[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:323
+                                format::DefaultFallbackStyle, Code,
+                                FSProvider.getFileSystem(llvm::None).get());
   if (!Style)
----------------
nit: I think we should have /*CWD=*/ on the None (and elsewhere)


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:402
+          InpAST->Inputs.FSProvider
+              ->getFileSystem(InpAST->Inputs.CompileCommand.Directory)
+              .get());
----------------
seems weird that we provide a working directory sometimes but not other times when getting format style.
I think this is because not providing one is suspicious, it's not actually needed, but sometimes we have one around?

Maybe just make getFormatStyleForFile take a FSProvider instead?


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:555
   //     different code layout.
   if (auto CorrespondingFile = getCorrespondingHeaderOrSource(
+          std::string(Path), FSProvider.getFileSystem(llvm::None)))
----------------
we could plumb in threadsafefs here too...


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:424
+  auto VFS = Baseline.StatCache->getConsumingFS(
+      Modified.FSProvider->getFileSystem(Modified.CompileCommand.Directory));
   // First scan preprocessor directives in Baseline and Modified. These will be
----------------
I don't see a corresponding removed CD - is this a bugfix?


================
Comment at: clang-tools-extra/clangd/support/FSProvider.cpp:81
+  if (FS->setCurrentWorkingDirectory(CWD))
+    elog("Failed to set CWD in RealFileSystemProvider");
+  return FS;
----------------
Nit: RealFileSystemProvider isn't the current class.

What about `elog("VFS: failed to set CWD to {0}: {1}", CWD, Err.message())`?


================
Comment at: clang-tools-extra/clangd/support/FSProvider.h:32
   virtual llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
-  getFileSystem() const = 0;
+  getFileSystem(llvm::NoneType CWD) const = 0;
+
----------------
/// Initial working directory is arbitrary.


================
Comment at: clang-tools-extra/clangd/support/FSProvider.h:34
+
+  /// Similar to above one, except it will try to set curret working directory
+  /// to \p CWD.
----------------
"Similar to above one" seems a bit awkward, maybe "As above"?

curret -> current


================
Comment at: clang-tools-extra/clangd/support/FSProvider.h:38
+  /// StringRef conversion possible.
+  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
+  getFileSystem(PathRef CWD) const;
----------------
did you want this to be virtual so that a truly virtual FS e.g. where the working directory is just a variable can be constructed with it directly?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81920/new/

https://reviews.llvm.org/D81920