[PATCH] D44272: [clangd] Support incremental document syncing
Simon Marchi via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 16 11:39:47 PDT 2018
simark marked 3 inline comments as done.
simark added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:101
json::obj{
- {"textDocumentSync", 1},
+ {"textDocumentSync", 2},
{"documentFormattingProvider", true},
----------------
ilya-biryukov wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > A comment mentioning that 2 means incremental document sync from LSP would be useful here.
> > I opted to add a TextDocumentSyncKind enum to Protocol.h and use that. Though instead of an enum, I did a class with static const fields:
> >
> > ```
> > struct TextDocumentSyncKind {
> > /// Documents should not be synced at all.
> > static const int None = 0;
> >
> > /// Documents are synced by always sending the full content of the document.
> > static const int Full = 1;
> >
> > /// Documents are synced by sending the full content on open. After that
> > /// only incremental updates to the document are send.
> > static const int Incremental = 2;
> > };
> > ```
> >
> > otherwise it would've required an (int) cast when using it to generate JSON:
> >
> > {"textDocumentSync", TextDocumentSyncKind::Incremental},
> I'd opt for a real enum. Having some extra type-safety and compiler warnings (i.e. non-covered enum value in switch) is much more valuable than a small inconvenience when converting the value back to json and back.
Makes sense.
================
Comment at: clangd/ClangdLSPServer.cpp:149
void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) {
if (Params.contentChanges.size() != 1)
return replyError(ErrorCode::InvalidParams,
----------------
ilya-biryukov wrote:
> 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?
I don't know of any client that does that. AFAIK, vscode doesn't:
https://github.com/Microsoft/vscode-languageserver-node/blob/master/client/src/client.ts#L907-L908
It's the only place I could find where a `DidChangeTextDocumentParams` is created, and it comes from a single `TextDocumentChangeEvent`, so there will always be only one item in the array.
================
Comment at: clangd/ClangdServer.cpp:557
// 64-bit unsigned integer.
if (Version < LastReportedDiagsVersion)
return;
----------------
ilya-biryukov wrote:
> 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.
Ok, thanks.
================
Comment at: clangd/DraftStore.cpp:47
+DocVersion DraftStore::updateDraft(PathRef File, StringRef Contents,
+ llvm::Optional<Range> range,
+ std::string *NewContents) {
----------------
ilya-biryukov wrote:
> 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 :-))
The .clang-tidy file in the root of the llvm repo contains it. But the one in the clang repo overrides the `readability-identifier-naming` section without defining `ParameterCase`. That's probably why it's not enabled.
```
'llvm-header-guard' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'llvm-include-order' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'llvm-namespace-comment' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'llvm-twine-local' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'misc-definitions-in-headers' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'misc-misplaced-const' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'misc-new-delete-overloads' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'misc-non-copyable-objects' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'misc-redundant-expression' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'misc-static-assert' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'misc-throw-by-value-catch-by-reference' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'misc-unconventional-assign-operator' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'misc-uniqueptr-reset-release' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'misc-unused-alias-decls' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'misc-unused-using-decls' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
'readability-identifier-naming' is enabled in the /home/emaisin/src/llvm/tools/clang/tools/extra/clangd/.clang-tidy.
```
So we should probably update the .clang-tidy file in the clang repo?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44272
More information about the cfe-commits
mailing list