jvikstrom added inline comments.
================ Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1110 PathRef File, std::vector<HighlightingToken> Highlightings) { + llvm::ArrayRef<HighlightingToken> Prev; + { ---------------- hokein wrote: > this seems unsafe, we get a reference of the map value, we might access it > without the mutex being guarded. > > ``` > std::vector<HighlightingToken> Old; > { > std::lock_guard<std::mutex> Lock(HighlightingsMutex); > Old = std::move(FileToHighlightings[File]); > } > ``` Aren't the parsing callbacks thread safe in the same file? I think @sammccall said so above. Although actually, the map might reallocate and move around the memory if things are inserted/deleted invalidating the reference maybe (I have no idea of how StringMap memory allocation works). So I guess it doesn't hurt to be safe and copy it. ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:255 + // ArrayRefs to the current line in the highlights. + ArrayRef<HighlightingToken> NewLine(Highlightings.data(), + Highlightings.data()), ---------------- hokein wrote: > IIUC, we are initializing an empty ArrayRef, if so, how about using > `NewLine(Highlightings.data(), /*length*/0)`? `NewLine(Highlightings.data(), > Highlightings.data())` is a bit weird, it took me a while to understand the > purpose. I couldn't do that because the `ArrayRef(const T *data, size_t length)` and `ArrayRef(const T *begin, const T *end)` were ambiguous. Added a cast to cast 0 to a size_t which solved it. 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