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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jul 29 12:02:23 PDT 2019

ilya-biryukov added a comment.

Mostly final NITs from me, but there is an important one about removing the `erase` call on `didOpen`.

Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:467
+    std::lock_guard<std::mutex> Lock(HighlightingsMutex);
+    FileToHighlightings.erase(File);
+  }
Now that the patch landed, this is obsolete. Could you remove?

Comment at: clang-tools-extra/clangd/ClangdServer.cpp:80
+    FileID MainFileID = SM.getMainFileID();
+    unsigned int FileSize = SM.getFileEntryForID(MainFileID)->getSize();
+    int NLines = AST.getSourceManager().getLineNumber(MainFileID, FileSize);
NIT: use `unsigned` instead of `unsigned int`

Comment at: clang-tools-extra/clangd/ClangdServer.h:56
+  virtual void onHighlightingsReady(
+      PathRef File, std::vector<HighlightingToken> Highlightings, int NLines) {}
NIT: could you document `NLines`

Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:294
+  // ArrayRefs to the current line in the highlights.
+  ArrayRef<HighlightingToken> NewLine(New.begin(),
+                                      /*length*/0UL);
Could we try to find a better name for this? Having `Line` and `NextLine()` which represent line numbers and `NewLine` which represents highlightings, creates some confusion.

Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:310
+  // highlightings beyond the end of the file. That might crash the client.
+  for (int Line = 0; Line != std::numeric_limits<int>::max() && Line < NLines;
+       Line = NextLine()) {
`Line != intmax` is redundant (NLines is <= intmax by definition).
Maybe remove it altogether to simplify the loop condition?

Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:80
+diffHighlightings(ArrayRef<HighlightingToken> New,
+                  ArrayRef<HighlightingToken> Old, int NLines);
Could you document what `NLines` is? Specifically, whether it's the number of lines for `New` or `Old`.

Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:60
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode) {
+  Annotations OldTest(OldCode);
NIT: add documentation on how this should be used
Most importantly, the fact that we need to put `^` on all changed lines should be mentioned.

Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:66
+  std::map<int, std::vector<HighlightingToken>> ExpectedLines;
+  for (const Position &Point : NewTest.points()) {
NIT: use `llvm::DenseMap`

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list