[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:332
 
+  ClangdServerOpts.SemanticHighlighting =
+      Params.capabilities.SemanticHighlighting;
----------------
I'm a bit nervous -- we will turn on this feature when the client states that it supports semantic highlighting, client may disable its reg-based highlighting, and relies on clangd's, but the feature in clangd is in pretty early stage...

I think we'd need to add a new command-line flag to clangd (disabled by default).


================
Comment at: clang-tools-extra/clangd/Protocol.h:1179
 
+// Contains all highlightings in a single line.
+struct SemanticHighlightingInformation {
----------------
please use `///` which is doxygen comment.

And `/// Represents a semantic highlighting information that has to be applied on a specific line of the text document.`


================
Comment at: clang-tools-extra/clangd/Protocol.h:1182
+  // The line these highlightings belong to.
+  unsigned int Line;
+  // The base64 encoded string of highlighting tokens.
----------------
nit: using `int` is fine here.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1189
+
+struct SemanticHighlightingParams {
+  // The textdocument these highlightings belong to.
----------------
Add a comment: `/// Parameters for the semantic highlighting (server-side) push notification.`


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:120
+std::vector<SemanticHighlightingInformation>
+highlightingTokensToSemanticHighlightingInformation(
+    llvm::ArrayRef<HighlightingToken> Tokens) {
----------------
add unittests for this function?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:132
+  llvm::raw_svector_ostream OS(LineHighlights);
+  for (const auto &Token : Tokens) {
+    if (Token.R.start.line != LastLine) {
----------------
the implementation requires Tokens sorted, maybe consider using a map to the group-by?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:152
+  std::map<HighlightingKind, std::vector<std::string>> Scopes = {
+      {HighlightingKind::Variable, {"variable.cpp"}},
+      {HighlightingKind::Function, {"entity.name.function.cpp"}}};
----------------
thinking more about this, the `.cpp` suffix is language-specific, clangd also supports other C-family languages (e.g. C, ObjC), so we may need corresponding language suffix in the scopes (based on the language mode).

No need to do the change here, but please add a FIXME.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:8
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHTING_H
----------------
The semantic highlighting is not in LSP yet, I think we need some documentations here to explain some more details about this feature in clangd, like the implementations are based on the proposal (https://github.com/microsoft/vscode-languageserver-node/pull/367).  


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:19
 enum class HighlightingKind {
   Variable,
   Function,
----------------
as this kind is used as the lookup table index, let's add `Variable = 0`


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:37
+// SemanticHighlightKind indexes correctly into this vector.
+std::vector<std::vector<std::string>> getSemanticTextMateScopes();
+
----------------
I'd name it `textMateScopeLookupTable`


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:40
+// Converts a vector of HighlightingTokens to a vector of
+// SemanticHighlightingInformation. Assumes the HighlightingToken's vector is
+// ordered in ascending order by line and secondarily by column of the token's
----------------
the assumption seems too strict here. IIUC, LSP doesn't require the order of the `SemanticHighlightingInformation[]`, would be nice to return an ordered one (but that's implementation detail). 


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:46
+std::vector<SemanticHighlightingInformation>
+highlightingTokensToSemanticHighlightingInformation(
+    llvm::ArrayRef<HighlightingToken> Tokens);
----------------
just `toSemanticHighlightingInformation`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63919