[PATCH] D40450: [clangd] Refactoring of GlobalCompilationDatabase

[Ilya Biryukov via Phabricator via cfe-commits]

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