[PATCH] D44272: [clangd] Support incremental document syncing

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:149
 void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) {
   if (Params.contentChanges.size() != 1)
     return replyError(ErrorCode::InvalidParams,
----------------
simark wrote:
> ilya-biryukov wrote:
> > We should handle more than a single change here.
> Ok, I'll try that.  I am not sure I interpret the spec correctly.  If you have two changes, the second change applies to the state of the document after having applied the first change, is that right?  If that's the case, I think we only need to iterate on the changes and call addDocument on them sequentially (which could be done in the DraftMgr, given the interface change to DraftStore you suggest lower).
I am interpreting it in the same manner, i.e. we need to apply the edits in the sequence they were provided.
But I would double-check on a client that actually send multiple changes. Does VSCode do that?


================
Comment at: clangd/ClangdServer.cpp:557
     // 64-bit unsigned integer.
     if (Version < LastReportedDiagsVersion)
       return;
----------------
simark wrote:
> ilya-biryukov wrote:
> > When you'll try remove the `DraftMgr`, this piece of code will be hard to refactor because it relies on `DocVersion`.
> > This is a workaround for a possible race condition that should really be handled by TUScheduler rather than this code, but we haven't got around to fixing it yet. (It's on the list, though, would probably get in next week).
> > 
> > A proper workaround for now would be to add `llvm::StringMap<uint64_t> InternalVersions` field to `ClangdServer`, increment those versions on each call to `addDocument` and use them here in the same way we currently use DraftMgr's versions.
> Hmm ok, it will probably make sense when I try to do it.  The `InternalVersions` map maps URIs to the latest version seen?
> 
> Is the race condition because we could get diagnostics for a stale version of the document, so we don't want to emit them?
For the record:
The race only happens if you add a file(version 1), remove the same file(version 2), add the same file again(version 3).
The diagnostics callbacks for version 1 and version 3 are triggered concurrently and we right report (3) and later (1).
It breaks "eventual consistency", i.e. clangd should eventually provide diagnostics for the **latest** version of the file, and we provide stale diagnostics in that case.


================
Comment at: clangd/DraftStore.cpp:47
+DocVersion DraftStore::updateDraft(PathRef File, StringRef Contents,
+                                   llvm::Optional<Range> range,
+                                   std::string *NewContents) {
----------------
simark wrote:
> ilya-biryukov wrote:
> > NIT: LLVM uses `UpperCamelCase` for parameters and local variables
> Woops.  I should learn to use clang-tidy.  It found other instances (the local variables) but it doesn't find the parameters not written in camel case.  Do you have an idea why?  I dumped the config and see these:
> 
> ```
>   - key:             readability-identifier-naming.ClassCase
>     value:           CamelCase
>   - key:             readability-identifier-naming.EnumCase
>     value:           CamelCase
>   - key:             readability-identifier-naming.UnionCase
>     value:           CamelCase
>   - key:             readability-identifier-naming.VariableCase
>     value:           CamelCase
> ```
> 
> I assume there must be a `ParamCase` or something like that, but I can't find the exhaustive list of parameters for that check.
There is indeed a `readability-idenfitier.ParameterCase` key.
Found in the code, it is probably missing from the docs. [[ https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/readability/IdentifierNamingCheck.cpp#L67 | IdentifierNamingCheck.cpp ]].
We should probably update the relevant clang-tidy config. (not sure where it is, though, I don't use clang-tidy for clangd development. should really start doing that :-))


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272