[PATCH] D51725: Allow un-setting the compilation database path

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D51725#1233254, @simark wrote:

> I am not sure I understand correctly your last point.  Of course, when restarting clangd, we need to send again a `didOpen` for each file the user has currently open in the editor.  But that should be  it?


Yes, this and any initial configuration settings.
My personal experience is from VSCode, which does not call `addDocument` IIUC. This results in clangd producing errors about running actions in non-opened files.

>> If you'll decide to go with an option to reset the path, see the comment about making empty path special. `Optional<Optional<>>` does not play nicely with json serialization code and hard to read (even though it does look like the right thing to do from the type system perspective).
> 
> Yes, noted.  I preferred to avoid giving a special meaning to an empty string because we could avoid it.  But I wouldn't mind changing it if we go that route.

Also not a big fan of it, but `Optional<Optional<>>` looks terrible. Maybe I'm missing an obvious pattern to make it simpler here, though.
WDYT about deserializing `null` to empty string? It would mean that specifying both `null` and `""` in json configuration yields the same results, but JSON that we send would look a bit nicer.



================
Comment at: clangd/Protocol.h:368
+  /// both Optionals are instantiated.
+  llvm::Optional<llvm::Optional<std::string>> compilationDatabasePath;
 
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > Not a big fan or something like this, but maybe give special meaning to empty path instead of wrapping an optional into an optional?
> > 
> > Double optionals are a real pain to write and read.
> Sorry just passing by since I've seen a similar usage that already exists in codebase, wanted to point it out.
> 
> Some api like this could be easier, to make the decision of using empty path or anything else for an invalid path and making it an implementation detail.
> 
> https://github.com/llvm-mirror/clang/blob/master/include/clang/Sema/DeclSpec.h#L53
It makes it less straight-forward, though.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725