ilya-biryukov added a comment.
A few suggestions from me, I haven't looked into the diffing algorithm and the
tests yet.
================
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);
----------------
Should can't we handle this on `didClose` instead?
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1115
+ std::lock_guard<std::mutex> Lock(HighlightingsMutex);
+ Old = FileToHighlightings[File];
+ }
----------------
hokein wrote:
> use std::move() to save a copy here. we'd drop the old highlighting anyway
> (replace it with new highlightings).
NIT: maybe std::move() into Old?
If we had exceptions, that would be complicated, but we don't have them
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1124
+ {
+ std::lock_guard<std::mutex> Lock(HighlightingsMutex);
+ FileToHighlightings[File] = std::move(Highlightings);
----------------
To follow the same pattern as diagnostics, could we store before calling
`publish...`?
================
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
----------------
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)
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:79
+std::vector<LineHighlightings>
+diffHighlightings(ArrayRef<HighlightingToken> NewHighlightings,
+ ArrayRef<HighlightingToken> OldHighlightings);
----------------
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?
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
