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

[Johan Vikström via Phabricator via cfe-commits]

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