jvikstrom marked 6 inline comments as done.
jvikstrom added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:458
+ // edit there are stale previous highlightings.
+ std::lock_guard<std::mutex> Lock(HighlightingsMutex);
+ FileToHighlightings.erase(File);
----------------
ilya-biryukov wrote:
> Should can't we handle this on `didClose` instead?
We are removing in didClose but the problem is that there is a race condition
(I think).
If someone does some edits and closes the document right after, the
highlightings for the final edit might finish being generated after the
FileToHighlightings have earsed the highlightings for the file. So next time
when opening the file we will have those final highlightings that were
generated for the last edit in the map.
I don't really know how we could handle this in didClose without having another
map and with an open/closed bit and checking that every time we generate new
highlightings. But we'd still have to set the bit to be open every didOpen so I
don't really see the benefit.
However I've head that ASTWorked is synchronous for a single file, is that the
case for the didClose call as well? Because in that case there is no race
condition.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:276
+ ArrayRef<HighlightingToken> OldHighlightings) {
+ // FIXME: There's an edge case when tokens span multiple lines. If the first
+ // token on the line started on a line above the current one and the rest of
----------------
ilya-biryukov wrote:
> ilya-biryukov wrote:
> > Do you have any ideas on how we can fix this?
> >
> > I would simply split those tokens into multiple tokens of the same kind at
> > newlines boundaries, but maybe there are other ways to handle this.
> >
> > In any case, could you put a suggested way to fix this (in case someone
> > will want to pick it up, they'll have a starting point)
> NIT: add assertions that highlightings are sorted.
Yeah, I would split the tokens into multiple lines as well. Or enforce that a
token is single line and handle it in addToken in the collector. (i.e. change
the HighlightingToken struct)
It's a bit annoying to solve because we'd have to get the line lengths of every
line that the multiline length token covers when doing that.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:79
+std::vector<LineHighlightings>
+diffHighlightings(ArrayRef<HighlightingToken> NewHighlightings,
+ ArrayRef<HighlightingToken> OldHighlightings);
----------------
ilya-biryukov wrote:
> Are we locked into the line-based diff implementation?
> It works nicely while editing inside the same line, but seem to do a bad job
> on a common case of inserting/removing lines.
>
> Does the protocol have a way to communicate this cleanly?
We are looked into some kind of line based diff implementation as the protocol
sends token line by line.
There should be away of solving the inserting/removing lines, but I'll have a
look at that in a follow up.
Theia seems to do a good job of moving tokens to where they should be
automatically when inserting a new line though. I want to be able to see how
vscode handles it first as well though before.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64475/new/
https://reviews.llvm.org/D64475
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
