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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits