ilya-biryukov requested changes to this revision.
ilya-biryukov added inline comments.
This revision now requires changes to proceed.
================
Comment at: clangd/ClangdLSPServer.cpp:101
json::obj{
- {"textDocumentSync", 1},
+ {"textDocumentSync", 2},
{"documentFormattingProvider", true},
----------------
A comment mentioning that 2 means incremental document sync from LSP would be
useful here.
================
Comment at: clangd/ClangdLSPServer.cpp:149
void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params)
{
if (Params.contentChanges.size() != 1)
return replyError(ErrorCode::InvalidParams,
----------------
We should handle more than a single change here.
================
Comment at: clangd/ClangdServer.h:159
/// constructor will receive onDiagnosticsReady callback.
void addDocument(PathRef File, StringRef Contents,
+ WantDiagnostics WantDiags = WantDiagnostics::Auto,
----------------
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.
================
Comment at: clangd/DraftStore.cpp:47
+DocVersion DraftStore::updateDraft(PathRef File, StringRef Contents,
+ llvm::Optional<Range> range,
+ std::string *NewContents) {
----------------
NIT: LLVM uses `UpperCamelCase` for parameters and local variables
================
Comment at: clangd/DraftStore.cpp:57
+ if (!Entry.Draft.hasValue()) {
+ log(llvm::Twine(
+ "Trying to do incremental update on draft without content: ") +
----------------
We should return an error to the client in that case. Changing return type to
`llvm::Expected<DocVersion>` and handling an error in the callers should do the
trick.
================
Comment at: clangd/DraftStore.cpp:66
+
+ newContents = Entry.Draft->substr(0, startIndex);
+ newContents += Contents;
----------------
We need to check that `startIndex` and `endIndex` are inside the bounds of the
string.
================
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);
----------------
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.
================
Comment at: clangd/Protocol.h:289
+ llvm::Optional<Range> range;
+
+ /// The new text of the range/document.
----------------
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.
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
