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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:255
+  // ArrayRefs to the current line in the highlights.
+  ArrayRef<HighlightingToken> NewLine(Highlightings.data(),
+                                      Highlightings.data()),
----------------
jvikstrom wrote:
> 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.
you could also do it like `NewLine(Highlightings.data(), /*length*/0UL);`, which saves you a `static_cast<>`.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:281
+      )cpp"}},
+                {{5},
+                 {
----------------
so the empty lines are stored separately, which is not easy to figure out from the testing code snippet. I think we should make the empty lines more obvious in the code snippet (maybe use an `Empty` annotations?) , and explicitly verify the empty lines.

What do you think refining the test like below, just annotate the diff tokens? I find it is easy to spot the diff tokens.

```
struct Testcase {
   Code Before;
   Code After;
};

std::vector<TestCase> cases = {
   { 
       R"cpp(
             int a;
             int b; 
             int c;
      )cpp",
      R"cpp(
            int a;
            $Empty[[]] // int b
            int $Variable[[C]];
      )cpp"
  }
}

oldHighlightings = getSemanticHighlightings(OldAST);
newHighlightings = getSemanticHighlightings(NewAST);
diffHighlightings = diff...;
// verify the diffHighlightings has the expected empty lines ("Empty" annotations).
// verify the diffHighlightings has the expected highlightings (the regular annotations);
```



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64475/new/

https://reviews.llvm.org/D64475