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: > NIT: maybe just inline `NewEnd` and `OldEnd` to save a few lines? Wouldn't that violate code style: https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop ? Or maybe that doesn't matter? (I have no preference either way, just picked this way as I could remeber reading from the styleguide) ================ Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:307 +TEST(SemanticHighlighting, HighlightingDiffer) { + struct { ---------------- 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. 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
