[PATCH] D81720: [clangd] Change prepareCompilerInstance to take an FSProvider

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Yay for getting rid of more VFSes passed around. Not sure about some of the other signature changes to prepareCompilerInstance. It seems a bit harder to call now.

In particular I'm not sure whether moving the preamble-stat-cache across setup from caller to callee is really an improvement once we modify that to wrap a ThreadsafeFS instead of a FS.
It's just... a reminder to use the cache, I guess? But the , nullptr, nullptr is annoying where we don't care about that.



================
Comment at: clang-tools-extra/clangd/Compiler.cpp:104
+    const PreambleFileStatusCache *FSCache) {
+  assert(FSProvider && "FSProvider is null");
   assert(!CI->getPreprocessorOpts().RetainRemappedFileBuffers &&
----------------
why is this a reference if it can't be null?

(otherwise, remove the assert message - just repeats the assertion)


================
Comment at: clang-tools-extra/clangd/Compiler.h:78
 std::unique_ptr<CompilerInstance> prepareCompilerInstance(
-    std::unique_ptr<clang::CompilerInvocation>, const PrecompiledPreamble *,
-    std::unique_ptr<llvm::MemoryBuffer> MainFile,
-    IntrusiveRefCntPtr<llvm::vfs::FileSystem>, DiagnosticConsumer &);
+    std::unique_ptr<clang::CompilerInvocation> CI,
+    std::unique_ptr<llvm::MemoryBuffer> Buffer, DiagnosticConsumer &DiagsClient,
----------------
none of the new parameter names say anything beyond repeating the types, can we drop them?


================
Comment at: clang-tools-extra/clangd/Compiler.h:79
+    std::unique_ptr<clang::CompilerInvocation> CI,
+    std::unique_ptr<llvm::MemoryBuffer> Buffer, DiagnosticConsumer &DiagsClient,
+    const tooling::CompileCommand &Cmd, const FileSystemProvider *FSProvider,
----------------
nit: grouping of parameters seems a bit confusing, preamble + mainfile together (in either order) was clearer I think. DiagnosticConsumer is logically an output param, nice to keep it at the end. Why split the FSProvider and the FS cache, etc.


================
Comment at: clang-tools-extra/clangd/Compiler.h:80
+    std::unique_ptr<llvm::MemoryBuffer> Buffer, DiagnosticConsumer &DiagsClient,
+    const tooling::CompileCommand &Cmd, const FileSystemProvider *FSProvider,
+    const PrecompiledPreamble *Preamble,
----------------
It doesn't seem reasonable to require both CompilerInvocation and CompileCommand. We can pass working dir in if we must...

(CompilerInvocation actually has a working directory field, but it looks like it potentially changes behavior. We could consider setting it in buildCompilerInvocation() in future...)


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:356
   llvm::Optional<IncludeFixer> FixIncludes;
-  auto BuildDir = VFS->getCurrentWorkingDirectory();
-  if (Inputs.Opts.SuggestMissingIncludes && Inputs.Index &&
-      !BuildDir.getError()) {
-    auto Style = getFormatStyleForFile(Filename, Inputs.Contents, VFS.get());
+  // FIXME: Why not use CompileCommand.Directory instead?
+  if (Inputs.Opts.SuggestMissingIncludes && Inputs.Index) {
----------------
you appear to have added the fixme and... also fixed it


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:273
+                              /*Preamble=*/nullptr,
+                              /*FSCache=*/nullptr);
   if (!Clang)
----------------
hmm!


================
Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:78
+      Diags, PI.CompileCommand, PI.FSProvider, &BaselinePreamble->Preamble,
+      BaselinePreamble->StatCache.get());
   PreprocessOnlyAction Action;
----------------
why this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81720