hokein 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:
> 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.
The race condition of highlighting sounds bad (especially when a user opens a
large file and closes it immediately, then the highlighting is finished and we
emit it to the client). No need to fix it in this patch, just add a FIXME.
Can we use the same mechanism for Diagnostic to guard the highlighting here? or
use the `DiagsMu` and `ReportDiagnostics` to guard the `callback.onMainAST()`
as well (which is probably not a good idea)...
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:307
+TEST(SemanticHighlighting, HighlightingDiffer) {
+ struct {
----------------
jvikstrom wrote:
> ilya-biryukov wrote:
> > Can we test this in a more direct manner by specifying:
> > 1. annotated input for old highlightings,
> > 2. annotated input for new highlightings,
> > 3. the expected diff?
> >
> > The resulting tests don't have to be real C++ then, allowing to express
> > what we're testing in a more direct manner.
> > ```
> > {/*Old*/ "$Variable[[a]]", /*New*/ "$Class[[a]]", /*Diff*/ "{{/*line */0,
> > "$Class[[a]]"}}
> > ```
> >
> > It also seems that the contents of the lines could even be checked
> > automatically (they should be equal to the corresponding line from
> > `/*New*/`, right?), that leaves us with even simpler inputs:
> > ```
> > {/*Old*/ "$Variable[[a]]", /*New*/ "$Class[[a]]", /*DiffLines*/ "{0}}
> > ```
> That's a great idea on how to write these tests.
hmm, I have a different opinion here (I'd prefer the previous one), let's chat.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64475/new/
https://reviews.llvm.org/D64475
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
