[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

This revision was automatically updated to reflect the committed changes. Closed by commit rL367521: [clangd] Duplicate lines of semantic highlightings sent removed. (authored by jvikstrom, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

hokein accepted this revision. hokein added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1125 +Old = std::move(FileToHighlightings[File]); +FileToHighlightings[File] = Highlightings; + } ilya-biryukov wrote: > jvikstrom

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

ilya-biryukov accepted this revision. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. LGTM from my side Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1125 +Old = std::move(FileToHighlightings[File]); +FileToHighlightings[File]

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom marked an inline comment as done. jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1125 +Old = std::move(FileToHighlightings[File]); +FileToHighlightings[File] = Highlightings; + } hokein wrote: >

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

hokein added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1125 +Old = std::move(FileToHighlightings[File]); +FileToHighlightings[File] = Highlightings; + } ilya-biryukov wrote: > NIT: avoid copying (from `Highlightings` into

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:408 +} + } // namespace ilya-biryukov wrote: > ilya-biryukov wrote: > > Could you also add a separate test that checks diffs when the number of > >

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 212528. jvikstrom marked an inline comment as done. jvikstrom added a comment. Copy outside lock. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/ https://reviews.llvm.org/D64475 Files:

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:408 +} + } // namespace ilya-biryukov wrote: > Could you also add a separate test that checks diffs when the number of lines > is getting smaller

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1125 +Old = std::move(FileToHighlightings[File]); +FileToHighlightings[File] = Highlightings; + } NIT: avoid copying (from `Highlightings` into the map) under

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 212522. jvikstrom marked 2 inline comments as done. jvikstrom added a comment. Stopped using expensive getLineNumber function. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

hokein added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:81 +unsigned FileSize = SM.getFileEntryForID(MainFileID)->getSize(); +int NLines = AST.getSourceManager().getLineNumber(MainFileID, FileSize); + from the comment of the

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:294 + // ArrayRefs to the current line in the highlights. + ArrayRef NewLine(New.begin(), + /*length*/0UL); ilya-biryukov wrote:

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 212301. jvikstrom marked 14 inline comments as done. jvikstrom added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/ https://reviews.llvm.org/D64475 Files:

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

hokein added a comment. a few more comments from my side. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:309 + // If the New file has fewer lines than the Old file we don't want to send + // highlightings beyond the end of the file. That might crash the

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

ilya-biryukov added a comment. Mostly final NITs from me, but there is an important one about removing the `erase` call on `didOpen`. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:467 +std::lock_guard Lock(HighlightingsMutex); +

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

ilya-biryukov added a comment. In D64475#1602195 , @jvikstrom wrote: > In D64475#1593481 , @ilya-biryukov > wrote: > > > The fix for a race condition on remove has landed in rL366577 > >

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom added a comment. In D64475#1593481 , @ilya-biryukov wrote: > The fix for a race condition on remove has landed in rL366577 > , this revision would need a small update > after it. Fixed to work with that

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 211890. jvikstrom marked 2 inline comments as done. jvikstrom added a comment. Made tests more readable. Updated to work with rL366577 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

ilya-biryukov added a comment. The fix for a race condition on remove has landed in rL366577 , this revision would need a small update after it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:307 +llvm::StringRef NewCode; +std::vector DiffedLines; + } TestCases[]{ @hokein rightfully pointed out that mentioning all changed lines

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

hokein added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:458 +// edit there are stale previous highlightings. +std::lock_guard Lock(HighlightingsMutex); +FileToHighlightings.erase(File); ilya-biryukov wrote: > jvikstrom

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:292 + /*length*/0UL); + auto NewEnd = NewHighlightings.end(); + auto OldEnd = OldHighlightings.end(); ilya-biryukov wrote: >

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 210613. jvikstrom added a comment. Removed one lock from onSemanticHighlighting. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/ https://reviews.llvm.org/D64475 Files:

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 210606. jvikstrom added a comment. Fixed test case that was actually (kinda) broken even though it didn't fail. (The last two edits in semantic-highlighting test. They removed the `= 2` part and did not insert an extra `;` which produced invalid code. It

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 210600. jvikstrom marked 7 inline comments as done. jvikstrom added a comment. Address comments and fixed bug where we'd send diffs outside of the file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 210601. jvikstrom added a comment. Remove braces. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/ https://reviews.llvm.org/D64475 Files: clang-tools-extra/clangd/ClangdLSPServer.cpp

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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 Lock(HighlightingsMutex); +

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:307 +TEST(SemanticHighlighting, HighlightingDiffer) { + struct { Can we test this in a more direct manner by specifying: 1. annotated input for

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 210567. jvikstrom edited the summary of this revision. jvikstrom added a comment. Added additional note to FIXME on how to solve and also move(ing) Old. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom marked 6 inline comments as done. jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:458 +// edit there are stale previous highlightings. +std::lock_guard Lock(HighlightingsMutex); +FileToHighlightings.erase(File);

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:274 +std::vector +diffHighlightings(ArrayRef NewHighlightings, + ArrayRef OldHighlightings) { NIT: maybe rename to `New` and `Old`, the suffix of

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

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

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

hokein added a comment. one missing point -- could you please update the patch description? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/ https://reviews.llvm.org/D64475 ___ cfe-commits

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. Thanks! Looks good from my side. @ilya-biryukov will be nice if you could take a second look on the patch. We plan to land it before the release cut today. Comment at:

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 210527. jvikstrom marked 9 inline comments as done. jvikstrom added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/ https://reviews.llvm.org/D64475 Files:

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

hokein added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:119 void addToken(SourceLocation Loc, const NamedDecl *D) { +if (D->isInvalidDecl()) + return; This means if we won't emit most of symbols if the AST is

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 210297. jvikstrom marked 6 inline comments as done. jvikstrom added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/ https://reviews.llvm.org/D64475 Files:

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

hokein added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:275 +bool operator<(const HighlightingToken , const HighlightingToken ) { + return std::tie(L.R, L.Kind) < std::tie(R.R, R.Kind);; +} nit: remove the redundant `;`.

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:281 + )cpp"}}, +{{5}, + { hokein wrote: > so the empty lines are stored separately, which is not easy to figure out

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 210282. jvikstrom marked 4 inline comments as done. jvikstrom added a comment. Rewrote test to be more readable. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/ https://reviews.llvm.org/D64475

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

hokein added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:255 + // ArrayRefs to the current line in the highlights. + ArrayRef NewLine(Highlightings.data(), + Highlightings.data()),

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 210253. jvikstrom marked 15 inline comments as done. jvikstrom added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/ https://reviews.llvm.org/D64475 Files:

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1110 PathRef File, std::vector Highlightings) { + llvm::ArrayRef Prev; + { hokein wrote: > this seems unsafe, we get a reference of the map value, we might access it

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

hokein added a comment. the code looks clearer now! Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:609 FixItsMap.erase(File); +std::lock_guard HLock(HighlightingsMutex); +FileToPrevHighlightings.erase(File); nit: I would create a new

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 210079. jvikstrom added a comment. Moved highlighting state to LSP layer. Removed class holding state. Addressed comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

sammccall added a comment. As discussed offline, it's a little scary that the highlight state for diffing lives in a separate map from TUScheduler's workers, but it's hard to fix without introducing lots of abstractions or layering violations. It does seem like a good idea to move the diffing

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

hokein added a comment. based on the offline discussion, now I understand the patch better (thanks). some comments on the interface. Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:60 + PathRef File, + std::vector>> Highlightings) + override;

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom marked an inline comment as done. jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:172 + const HighlightingToken ) { + return Lhs.R.start.line == Rhs.R.start.line + ?

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 209228. jvikstrom added a comment. Removed unused code snippets. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/ https://reviews.llvm.org/D64475 Files:

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom updated this revision to Diff 209225. jvikstrom marked 5 inline comments as done. jvikstrom added a comment. Made diffing function shorter, added multiple previous highlighting entries. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

sammccall added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:172 + const HighlightingToken ) { + return Lhs.R.start.line == Rhs.R.start.line + ? Lhs.R.start.character < Rhs.R.start.character

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

hokein added a comment. Thanks for bringing this up and implementing it. I haven't looked into the details of the patch, just some high-level comments: - the scope of the patch seems a bit unclear to me, what's the problem we are trying to solve? - the patch looks like calculating the

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

jvikstrom created this revision. jvikstrom added reviewers: hokein, sammccall, ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, mgrang, jkorous, MaskRay. Herald added a project: clang. Added a class for diffing highlightings and removing duplicate lines. Integrated into