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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

mostly good, a few more nits.



================
Comment at: clang-tools-extra/clangd/Protocol.h:1187
+};
+
+bool operator==(const SemanticHighlightingInformation &Lhs,
----------------
nit: remove the blank line to be consistent with the existing style in this file.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1199
+};
+
+llvm::json::Value toJSON(const SemanticHighlightingParams &Highlighting);
----------------
nit: remove this blank line.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:138
+  for (const auto &Line : TokenLines) {
+    llvm::SmallVector<char, 128> LineHighlights;
+    llvm::raw_svector_ostream OS(LineHighlights);
----------------
jvikstrom wrote:
> hokein wrote:
> > The code is a bit tricky here, if I understand the code correctly, `LineHighlights` is the binary data of tokens (each `char` represents a byte).
> > Maybe `LineEncodingTokens`?
> They aren't "fully" encoded yet though, they get encoded after the inner loop is done. How about `LineByteTokens`?
> 
sounds good.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:121
+toSemanticHighlightingInformation(llvm::ArrayRef<HighlightingToken> Tokens) {
+  if (Tokens.size() == 0) {
+    return {};
----------------
nit: remove the "{}"


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:138
+      // Writes the token to LineByteTokens in the byte format specified by the
+      // LSP proposal.
+      write32be(Token.R.start.character, OS);
----------------
add


```
|<--- 4 bytes  --->|<-2 bytes->|<-2 bytes->|
|    character      |  length       |    index       | 
```

to the comment, it helps reader to understand how the encode like.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:156
+  std::vector<std::vector<std::string>> NestedScopes(Scopes.size());
+  for (const auto &Scope : Scopes) {
+    NestedScopes[static_cast<int>(Scope.first)] = Scope.second;
----------------
nit: remove the `{}` for one-body statement.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:7
 //
+// An implementation of semantic highlighting based on this proposal:
+// https://github.com/microsoft/vscode-languageserver-node/pull/367 in clangd.
----------------
nit: this should in a new section:

```

//==-- SemanticHighlighting.h - Generating highlights from the AST-- C++ -*-==//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//===---------------------------------------------------------------------------------===//
//  
//  An implementation ....
// 
//===---------------------------------------------------------------------------------===//
```


================
Comment at: clang-tools-extra/clangd/test/initialize-params.test:3
 # Test initialize request parameters with rootUri
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"test:///workspace","capabilities":{},"trace":"off"}}
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"test:///workspace","capabilities":{"textDocument":{"semanticHighlightingCapabilities":{"semanticHighlighting":true}}},"trace":"off"}}
 #      CHECK:  "id": 0,
----------------
How about moving this to the new `semantic-highlighting.test`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63919