[PATCH] D53687: [clangd] Make in-memory CDB always available as an overlay, refactor.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 2 inline comments as done.
sammccall added a comment.

In https://reviews.llvm.org/D53687#1275580, @ilya-biryukov wrote:

> FWIW, the old implementation of the CDB looked simpler (which makes sense, since it only allowed the in-memory compile commands, but the new implementation also falls back to the base CDB, i.e. it's now doing two things).


Yeah, this is more complicated than the old `InMemoryCompilationDatabase`, but simpler than `InMemoryCompilationDatabase` + `ClangdLSPServer::CompilationDb` together, which it's replacing.



================
Comment at: clangd/ClangdLSPServer.cpp:670
+          /*Output=*/"");
+      if (Old != New)
+        CDB->setCompileCommand(File, std::move(New));
----------------
ilya-biryukov wrote:
> Is this an optimization to not trigger compile command changes?
> Can we perform it on the CDB level to make sure we hit it in the future if more code calling `setCompileCommand` is added?
> Is this an optimization to not trigger compile command changes?
This is my attempt to preserve the optimization from rL338597, without having to keep the more complex interface to setCompileCommand.
(And fix a bug in it: if there was an old command and you change it, you should refresh).

The optimisation seems kind of suspect to me to be honest (if it's worth doing, it's probably worth doing right - i.e. per-file, not globally), but it's easy enough to preserve for now.

> Can we perform it on the CDB level to make sure we hit it in the future if more code calling setCompileCommand is added?
Not quite sure what you mean: ClangdLSPServer needs to check whether there were changes in order to decide whether to invalidate. Having CompilationDatabase *also* aware of changes seems more complex/coupled and no less error-prone.


================
Comment at: clangd/ClangdLSPServer.h:42
   ClangdLSPServer(Transport &Transp, const clangd::CodeCompleteOptions &CCOpts,
-                  llvm::Optional<Path> CompileCommandsDir,
-                  bool ShouldUseInMemoryCDB, const ClangdServer::Options &Opts);
+                  llvm::Optional<Path> CompileCommandsDir, bool NoReadCDB,
+                  const ClangdServer::Options &Opts);
----------------
ilya-biryukov wrote:
> NIT: the negative variable names might cause confusion (i.e. the double negations are hard to read). Maybe use the name of the corresponding field (UseDirectoryCDB)?
I was reluctant to replace a bool constructor parameter with a bool that does the exact opposite, but we do only have one callsite...


================
Comment at: clangd/GlobalCompilationDatabase.h:79
+/// using an in-memory mapping.
+class OverlayCDB : public GlobalCompilationDatabase {
 public:
----------------
ilya-biryukov wrote:
> The name does not seem to fully capture what this class does, i.e. allowing to override compile commands for some files.
> Maybe use `OverridenCDB` or something similar?
Hmm, I feel the opposite way about these names - overlaying is exactly selective overriding of some elements, and overriding is more vague.

I did try to think of some other names, I almost like "mutable" but it's not actually essential, e.g. we don't allow the fallback flags to be mutated.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53687