ilya-biryukov added a comment.
Thanks again. This generally looks LGTM, just a last drop of small nits. Will
approve as soon as they land.
================
Comment at: clangd/DraftStore.cpp:75
+ return llvm::make_error<llvm::StringError>(
+ llvm::formatv("Range's end position (line={0}, character={1}) is "
+ "before start position (line={2}, character={3}).",
----------------
Could we add a format provide for `Position` to make this formatting code
shorter?
We'll need to define the corresponding specialization in `Protocol.h`:
```
namespace llvm {
template <>
struct format_provider<clang::clangd::Position> {
static void format(const clang::clangd::Position &Pos, raw_ostream &OS,
StringRef Style) {
assert(Style.empty() && "style modifiers for this type are not supported");
OS << Pos;
}
};
}
```
This will allow to simplify this call site to:
```
llvm::formatv("Range's end position ({0}) is before start position ({1})", End,
Start)
```
================
Comment at: clangd/DraftStore.cpp:100
+ EntryIt->second = Contents;
+ return std::move(Contents);
+}
----------------
Doing `return std::move(localvar)` is a pessimization. Use `return Contents;`
instead.
(for reference see
[[https://stackoverflow.com/questions/14856344/when-should-stdmove-be-used-on-a-function-return-value|this
SO post]], clang should give a warning for that case too)
================
Comment at: clangd/DraftStore.h:36
/// Replace contents of the draft for \p File with \p Contents.
- void updateDraft(PathRef File, StringRef Contents);
+ void addDraft(PathRef File, StringRef Contents);
+
----------------
simark wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > simark wrote:
> > > > ilya-biryukov wrote:
> > > > > Could we add versions from LSP's `VersionedTextDocumentIdentifier`
> > > > > here and make the relevant sanity checks?
> > > > > Applying diffs to the wrong version will cause everything to fall
> > > > > apart, so we should detect this error and signal it to the client as
> > > > > soon as possible.
> > > > I agree that applying diffs to the wrong version will break basically
> > > > everything, but even if we detect a version mismatch, I don't see what
> > > > we could do, since we don't have a mean to report the error to the
> > > > client. The only thing we could do is log it (which we already do.
> > > If we couldn't apply a diff, we should return errors from all future
> > > operations on this file until it gets closed and reopened. Otherwise
> > > clangd and the editor would see inconsistent contents for the file and
> > > results provided by clangd would be unreliable.
> > > The errors from any actions on the invalid file would actually be visible
> > > to the users.
> > >
> > > The simplest way to achieve that is to remove the file from `DraftStore`
> > > and `ClangdServer` on errors from `updateDraft`.
> > > This will give "calling action on non-tracked file" errors for future
> > > operations and the actual root cause can be figured out from the logs.
> > We still ignore version numbers from the LSP.
> > Is this another change that didn't get in?
> The more I think about it, the less sure I am that this is the intended usage
> of the version. The spec doesn't even say what the initial version of a file
> should be, 0 or 1? Then, it just says that the version optionally contained
> in the `VersionedTextDocumentIdentifier` reflects the version of the document
> after having applied the edit. But I don't think we can predict and validate
> what that version should be. Most clients will probably just use a sequence
> number, but I guess they don't have to...
>
> Also, I initially assumed that having N changes in the `contentChanges` array
> would mean that the version number would be bumped by N. But now that I
> re-read it, there's really nothing that says it should behave like this.
>
> I think that we should just record the version number and treat it as opaque,
> so that we can use it later when sending text edits to the client, for
> example. The client can then verify that the edit is for the same version of
> the document that it has at the moment.
>
> Therefore, I don't think it would really be useful in this patch.
The initial version is provided in `didOpen` (see `TextDocumentItem`).
I've also misread description of `contentChanges` in the same way. It's true
that versions might grow by any
[[https://github.com/Microsoft/language-server-protocol/issues/241|any positive
number]], so we can't rely on them for the checks I propose here.
Thanks for looking into that and explaining the spec :-)
================
Comment at: clangd/DraftStore.h:26
/// filenames. The contents are owned by the DraftStore.
class DraftStore {
public:
----------------
Could we also mention in the comment that this class now handles applying
incremental document changes?
================
Comment at: unittests/clangd/DraftStoreTests.cpp:36
+/// Send the changes one by one to updateDraft, verify the intermediate
results.
+static void stepByStep(llvm::ArrayRef<IncrementalTestStep> Steps) {
+ DraftStore DS;
----------------
NIT: another redundant static
================
Comment at: unittests/clangd/DraftStoreTests.cpp:62
+/// Send all the changes at once to updateDraft, check only the final result.
+static void allAtOnce(llvm::ArrayRef<IncrementalTestStep> Steps) {
+ DraftStore DS;
----------------
NIT: another redundant `static`
================
Comment at: unittests/clangd/DraftStoreTests.cpp:66
+ Annotations FinalSrc(Steps.back().Src);
+ const char Path[] = "/hello.cpp";
+ std::vector<TextDocumentContentChangeEvent> Changes;
----------------
NIT: another instance of `constexpr StringLiteral`
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44272
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
