[PATCH] D63919: [clangd] Emit publishSemanticHighlighting in LSP if enabled

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:333
+  ClangdServerOpts.SemanticHighlighting =
+      Params.capabilities.SemanticHighlighting;
   if (Params.rootUri && *Params.rootUri)
----------------
I'd put the logic here rather than in `ClangdServer.cpp`, just `ClangdServerOpts.SemanticHighlighting = ClangdServerOpts.HiddenFeatures &&  Params.capabilities.SemanticHighlighting;`.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:131
+    if (TokenLines.find(LineIdx) == TokenLines.end()) {
+      TokenLines[LineIdx] = {Token};
+    } else {
----------------
here we lookup the map twice, we could save one cost of lookup. 

`TokenLines[LindIdx].push_back(Token)` should work.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:143
+    for (const auto &Token : Line.second) {
+      write32be(Token.R.start.character, OS);
+      write16be(Token.R.end.character - Token.R.start.character, OS);
----------------
could we have some comments explaining the details about encoding Token here? 


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:144
+      write32be(Token.R.start.character, OS);
+      write16be(Token.R.end.character - Token.R.start.character, OS);
+      write16be(static_cast<int>(Token.Kind), OS);
----------------
if the token is across multiple line, we will emit an ill-formed results.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:44
+
+// Converts a vector of HighlightingTokens to a vector of
+// SemanticHighlightingInformation. The token string in
----------------
just: convert to LSP's semantic highlighting information.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:103
+  std::vector<HighlightingToken> Tokens{
+      HighlightingToken{HighlightingKind::Variable,
+                        Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
----------------
we could drop the explicit `HighlightingToken`, just 

```
... Tokens = {
   {HighlightingKind::Variable, Range {...}},
  {....}
}
```


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:104
+      HighlightingToken{HighlightingKind::Variable,
+                        Range{CreatePosition(3, 8), CreatePosition(3, 12)}},
+      HighlightingToken{HighlightingKind::Function,
----------------
`Range{ /*start*/{3, 8}, /*end*/{3, 12} }` should be compilable.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:112
+  std::vector<SemanticHighlightingInformation> Correct = {
+      SemanticHighlightingInformation{1, "AAAAAQAEAAA="},
+      SemanticHighlightingInformation{3, "AAAACAAEAAAAAAAEAAMAAQ=="}};
----------------
here as well, just `{ {1, "...." } }`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63919