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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits