ilya-biryukov marked an inline comment as done. ilya-biryukov 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); ---------------- jvikstrom wrote: > 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. You are correct, there is actually a race condition. We worked hard to eliminate it for diagnostics, but highlightings are going through a different code path in `ASTWorker`, not guarded by `DiagsMu` and `ReportDiagnostics`. And, unfortunately, I don't think this guard here prevents it in all cases. In particular, there is still a possibility (albeit very low, I guess) that the old highlightings you are trying to remove here are still being computed. If they are reported **after** this `erase()` runs, we will end up with inconsistent highlightings. Ideally, we would guard the diagnostics callback with the same mutex, but given our current layering it seems like a hard problem, will need to think what's the simplest way to fix this. ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:79 +std::vector<LineHighlightings> +diffHighlightings(ArrayRef<HighlightingToken> NewHighlightings, + ArrayRef<HighlightingToken> OldHighlightings); ---------------- jvikstrom wrote: > 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. It seems there no compact way to send interesting diffs then, although the clients can do a reasonably good job of moving the older version of highlightings on changes until the server sends them a new version. The diff-based approach only seems to be helping with changes on a single line. Which is ok, thanks for the explanation. 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
