[PATCH] D48071: [clangd] Add an option controlling caching of compile commands.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdLSPServer.h:38
                   llvm::Optional<Path> CompileCommandsDir,
-                  const ClangdServer::Options &Opts);
+                  const ClangdServer::Options &Opts, bool CacheCompileCommands);
 
----------------
sammccall wrote:
> (the options split looks a little odd from the outside. One could make an argument for inheriting ClangdLSPServer::Options from ClangdServer::Options and adding the compile-commands/code completion options there. No need to restructure anything in this patch though)
Options struct SG, even though is number of params is manageable, and we have just a single call of this ctor at the time.
I'm not a big fan of inheriting data structs, though, would rather use composition.


================
Comment at: clangd/ClangdLSPServer.h:105
+  // Can be null if no caching was requested.
+  std::unique_ptr<CachingCompilationDb> CachedCDB;
 
----------------
sammccall wrote:
> nit: any reason for unique_ptr over optional?
> (With optional, I think it's clear enough to remove the comment)
CachingCompilationDb is non-movable (because of std::mutex field), so we can't properly initialize it in ctor-initializers.
And we need it to pass it into `ClangdServer` in ctor-initializers, so it's hard to put the init code into the ctor body.
That all looks brittle, but I wouldn't refactor this in this patch.


================
Comment at: clangd/tool/ClangdMain.cpp:141
+                   "come from the compilation databases."),
+    llvm::cl::init(false), llvm::cl::Hidden);
+
----------------
sammccall wrote:
> init(true) to avoid changing behavior?
This is intentional, it seems `false` is a better default, given that all implementation in LLVM cache it all themselves.
We should set to true for our CDB that actually needs it. WDYT?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48071