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