[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: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 ill-formed? I'm not sure whether we should include it in this patch, it seems out of scope. could we leave it out in this patch?




================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:68
+
+/// Removes every line where \c Highlightings is the same as \c
+/// PrevHighlightings. If \c PrevHighlightings has lines that does not exist
----------------
The comment seems a bit out of date (as the code is updated during review), and it should mention the diff strategy of this function.

```
// Return a line-by-line diff between two highlightings.
//  - if the tokens on a line are the same in both hightlightings, we omit this line
//  - if a line exists in NewHighligtings but not in OldHighligtings, we emit the tokens on this line
//  - if a line not exists in NewHighligtings but in OldHighligtings, we emit an empty line (to tell client not highlighting this line)
``` 


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:53
   }
+  return ExpectedTokens;
+}
----------------
nit: let's sort the tokens here to make the result deterministic.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:59
+  std::vector<int> EmptyLines(EmptyRanges.size());
+  for (unsigned I = 0, End = EmptyRanges.size(); I < End; ++I)
+    EmptyLines[I] = EmptyRanges[I].start.line;
----------------
nit: you could use for-range loop to simplify the code.

```
for (const auto& EmptyRange : EmptyRanges)
   EmptyLines.push_back(EmptyRange.start.line);
```


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:75
+  std::vector<HighlightingToken> OldActualTokens = getActualTokens(OldTest);
+  std::vector<HighlightingToken> NewActualTokens = getActualTokens(NewTest);
+  std::vector<HighlightingToken> ExpectedTokens = getExpectedTokens(NewTest);
----------------
nit: since `OldActualTokens` and `NewActualTokens` are used only once in `diffHighlightings`, I'd inline them there (instead of creating two new variables).


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:88
+  for (auto &LineTokens : ExpectedLines) {
+    llvm::sort(LineTokens.second);
+    ExpectedLinePairHighlighting.push_back(
----------------
if we do sort in `getExpectedTokens`, we don't need this here.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:274
+  // The first entry is the old code. The second entry is the new code.
+  std::vector<std::pair<const char *, const char *>> TestCases{{
+      R"cpp(
----------------
nit: Use an explicit struct, then you wouldn't need a comment.

```
struct {
   llvm::StringRef OldCode;
   llvm::StringRef NewCode;
} TestCases[] = {
   ...
}
```


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:275
+  std::vector<std::pair<const char *, const char *>> TestCases{{
+      R"cpp(
+        int A
----------------
could we add one more case?

```
// Before
int a;
int b;
int c;

// After
int a;
int $Variable[[new_b]];
int c;
```


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:327
+
+  for (auto Test : TestCases) {
+    checkDiffedHighlights(Test.first, Test.second);
----------------
nit: `const auto&`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475