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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
