[PATCH] D49758: [clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdLSPServer.h:106
   CachingCompilationDb CDB;
+  InMemoryCompilationDb InMemoryCDB;
 
----------------
This code starts to be a little hard to follow. Could we extract it into an external class that encapsulates this logic? Something like:

```
class ClangdLSPServer {

private:
  class CompilationDB {
  public:
    static CompilationDB makeInMemory();
    stiatc CompilationDB makeDirectoryBased();
  
    void invalidate(PathRef File);
     /// Only valid for in-memory CDB, no-op and error log on DirectoryBasedCDB.
    void setCompileCommandsForFile(PathRef File);
    /// Only valid for directory-based CDB, no-op and error log on InMemoryCDB;
    void setExtraFlags(PathRef File);
    /// Returns a CDB that should be used to get compile commands for the current instance of ClangdLSPServer.
    GlobalCompilationDatabase& getCDB(); 
  
  private:
     // if IsDirectoryBased is true, an instance of InMemoryCDB.
     // If IsDirectoryBased is false, an instance of DirectoryBasedCDB.  unique_ptr<GlobalCompilationDatabase> CDB;
    unique_ptr<GlobalCompilationDatabase> CDB;
    // non-null only for directory-based CDB
    unique_ptr<CachingCompilationDatabase> CachingCDB;
    bool IsDirectoryBased;
  };
 
  CompilationDB CDB;
  // .....
}
```

We can static_cast to InMemoryCDB or DirectoryBasedCDB based on the IsDirectoryBased flag to implement all the operations we define in the helper class.


================
Comment at: clangd/GlobalCompilationDatabase.h:136
+  mutable std::mutex Mutex;
+  mutable llvm::StringMap<llvm::Optional<tooling::CompileCommand>>
+      Commands; /* GUARDED_BY(Mut) */
----------------
Maybe remove 'mutable' from Commands? We shouldn't need it.


================
Comment at: clangd/GlobalCompilationDatabase.h:136
+  mutable std::mutex Mutex;
+  mutable llvm::StringMap<llvm::Optional<tooling::CompileCommand>>
+      Commands; /* GUARDED_BY(Mut) */
----------------
ilya-biryukov wrote:
> Maybe remove 'mutable' from Commands? We shouldn't need it.
Maybe make the value type `CompileCommand` instead of `Optional<CompileCommand>`? 
We seem to only use the optional when inserting values and we could easily rewrite that code to use `map::insert()`.


================
Comment at: clangd/Protocol.h:425
+/// compilation database.
+struct ClangdConfigurationCompilationDatabaseUpdate {
+  std::string workingDirectory;
----------------
Maybe use a shorter name, e.g. `ClangdCompileCommand`? This class looks *almost* like `tooling::CompileCommand`, so having a similar name seems reasonable.



================
Comment at: clangd/tool/ClangdMain.cpp:170
+static llvm::cl::opt<bool>
+    InMemoryCompileCommands("in-memory-compile-commands",
+                            llvm::cl::desc("Use an in-memory compile commands"),
----------------
The important distinction seems to be where the compile commands come from, maybe make the name centered on the fact that compile commands are read from the protocol rather from the filesystem?

E.g. using named option values:
`--compile_args_from={lsp|filesystem}`
Or a boolean option:
`--compile_args_from_lsp`

WDYT?


================
Comment at: clangd/tool/ClangdMain.cpp:171
+    InMemoryCompileCommands("in-memory-compile-commands",
+                            llvm::cl::desc("Use an in-memory compile commands"),
+                            llvm::cl::init(false), llvm::cl::Hidden);
----------------
Maybe elaborate a bit more on this mode? E.g. `clangd will get compile all commands from via LSP and won't look at the filesystem for 'compile_commands.json' files`


https://reviews.llvm.org/D49758