ilya-biryukov added inline comments.
================ Comment at: clangd/ClangdLSPServer.cpp:254 + std::move(Primary), llvm::make_unique<FixedCompilationDatabase>( + ".", ArrayRef<std::string>{})); +} ---------------- Clangd still changes working directory when running the parsing, so `"."` might end up being a different dir on multiple runs over the same file. Maybe use file's parent for now? ================ Comment at: clangd/ClangdServer.cpp:178 + // Create an owned version of CDB to use ArgumentsAdjustingCompilations. + struct OwnedCDB : public tooling::CompilationDatabase { + OwnedCDB(const tooling::CompilationDatabase &Ref) : Ref(Ref) {} ---------------- Maybe move this code out of `ClangdServer`? Arguably, it should be less confusing for clients of `ClangdServer` if they get exactly the compile commands returned by `CDB`. This would also allow to get rid of `ResourceDir` parameter, which seems to be handled by `CompilationDatabase` now. ================ Comment at: clangd/ClangdServer.h:208 /// Various messages are logged using \p Logger. - ClangdServer(GlobalCompilationDatabase &CDB, + ClangdServer(const tooling::CompilationDatabase &CDB, DiagnosticsConsumer &DiagConsumer, ---------------- Are there any mutating methods in `CompilataionDatabase`? Why do we use `const &` instead of `&`? ================ Comment at: clangd/GlobalCompilationDatabase.cpp:57 + for (auto &Cmd : Commands) { + assert(Cmd.CommandLine.size() >= 2 && + "Expected at least the command and the filename"); ---------------- IIRC, `Cmd` could come directly from `compile_commands.json`. We should avoid crashing `clangd` if contents of `compile_commands.json` are invalid. Could we log the error instead and continue with other commands instead? ================ Comment at: clangd/GlobalCompilationDatabase.h:34 + std::vector<tooling::CompileCommand> + getCompileCommands(PathRef File) const override; ---------------- It seems that this `const` means more trouble for implementations (i.e. using `mutable`, etc.) without providing any value. Maybe we should consider removing it from the interface? Am I missing the upsides of using `const` here? ================ Comment at: clangd/GlobalCompilationDatabase.h:61 + llvm::StringMap<std::vector<std::string>> ExtraFlags; + std::unique_ptr<tooling::CompilationDatabase> Inner; +}; ---------------- Maybe we should consider using non-owning reference here? It gives more flexibility, e.g. allowing to allocate on stack or store `CDB` directly inside the class fields, albeit it requires a bit more code to setup. I'd say upsides of non-owning semantics outweigh a bit of added complexity https://reviews.llvm.org/D40450 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits