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