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

Johan Vikström via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jul 18 10:35:35 PDT 2019

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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list