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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
