ilya-biryukov added inline comments.

Comment at: clangd/ClangdLSPServer.h:38
                   llvm::Optional<Path> CompileCommandsDir,
-                  const ClangdServer::Options &Opts);
+                  const ClangdServer::Options &Opts, bool 
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 

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?

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to