simark marked 2 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:
> 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},
================
Comment at: clangd/ClangdLSPServer.cpp:149
void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params)
{
if (Params.contentChanges.size() != 1)
return replyError(ErrorCode::InvalidParams,
----------------
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).
================
Comment at: clangd/ClangdServer.cpp:557
// 64-bit unsigned integer.
if (Version < LastReportedDiagsVersion)
return;
----------------
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?
================
Comment at: clangd/ClangdServer.h:159
/// constructor will receive onDiagnosticsReady callback.
void addDocument(PathRef File, StringRef Contents,
+ WantDiagnostics WantDiags = WantDiagnostics::Auto,
----------------
ilya-biryukov wrote:
> I don't think we should do this on `ClangdServer` level. We will have to copy
> the whole file anyway before passing it to clang, so there are no performance
> wins here and it complicates the interface.
> I suggest we update the document text from diffs on `ClangdLSPServer` level
> and continue passing the whole document to `ClangdServer`.
> It would mean `DraftMgr` will need to move to `ClangdLSPServer`, but it's
> fine.
>
`ClangdServer` also needs `DraftMgr` for `forceReparse` and
`reparseOpenedFiles`. I guess that too would move to `ClangdLSPServer`? I'm
just not sure how to adapt the unit tests, since we don't have unittests using
`ClangdLSPServer` (yet).
================
Comment at: clangd/DraftStore.cpp:47
+DocVersion DraftStore::updateDraft(PathRef File, StringRef Contents,
+ llvm::Optional<Range> range,
+ std::string *NewContents) {
----------------
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.
================
Comment at: clangd/DraftStore.cpp:66
+
+ newContents = Entry.Draft->substr(0, startIndex);
+ newContents += Contents;
----------------
ilya-biryukov wrote:
> We need to check that `startIndex` and `endIndex` are inside the bounds of
> the string.
Right.
================
Comment at: clangd/DraftStore.h:60
/// \return The new version of the draft for \p File.
- DocVersion updateDraft(PathRef File, StringRef Contents);
+ DocVersion updateDraft(PathRef File, StringRef Contents,
+ llvm::Optional<Range> Range, std::string
*NewContents);
----------------
ilya-biryukov wrote:
> Let's model the LSP more closely here:
>
> ```
> // Protocol.h
> struct TextDocumentContentChangeEvent {
> llvm::Optional<Range> range;
> llvm::Optional<int> rangeLength;
> std::string text;
> };
>
> /// DraftManager
> class DraftManager {
> /// For initial didOpen.
> DocVersion addDraft(PathRef File, std::string Contents);
>
> /// For didChange, handles both incremental and full updates. \p Changes
> cannot be empty.
> DocVersion updateDraft(PathRef File,
> ArrayRef<TextDocumentContentChangeEvent> Changes);
> };
> ```
>
> Keeping track of LSP versions here could also be useful to make sure we drop
> any updates in between (which would definitely lead to surprising results),
> but it's ok if we add a FIXME and do it later.
By `DraftManager` I suppose you mean `DraftStore`? I'm fine with that
interface change.
================
Comment at: clangd/Protocol.h:289
+ llvm::Optional<Range> range;
+
+ /// The new text of the range/document.
----------------
ilya-biryukov wrote:
> Please add `rangeLength` from lsp spec too. On one hand, it seems redundant,
> but it can be used to add assertions that the text is the same.
Ok.
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
