[PATCH] D76663: [clangd] Support new semanticTokens request from LSP 3.16.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D76663#1942273 <https://reviews.llvm.org/D76663#1942273>, @sammccall wrote:

> For some context here, I was doing some digging as Heyward Fann is looking at hooking up our existing syntax highlighting to coc.nvim, and he and Jack Guo (of jackguo380/vim-lsp-cxx-highlight) were asking about the protocol.
>
> The new LSP protocol looks really solid, including better incremental highlight support than the Theia proposal, though this patch doesn't implement incremental yet. (In particular, no sending thousands of re-highlights when inserting a new line). It's also request-response rather than notification-based, which is easier to implement on the server side. Also the VSCode client-side of our highlighting feels like significant technical debt we could be rid of.
>
> So I think we should try to support the new LSP and drop the older Theia one ASAP (clangd 12?), even if semantic highlighting isn't a really high priority for us.


Thanks for the summary, sounds a good plan.

The patch is mostly good, just a few nits. I will try to play it with VSCode.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:597
+             llvm::json::Object{
+                 {"documentProvider", true},
+                 {"legend",
----------------
nit: shall we explicitly set the `rangeProvider` to false as we don't support the semantic token range request now.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1377
+};
+llvm::json::Value toJSON(const SemanticTokens &FStatus);
+
----------------
nit: FStatus => Tokens.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:449
+std::vector<SemanticToken>
+toSemanticTokens(llvm::ArrayRef<HighlightingToken> Tokens) {
+  std::vector<SemanticToken> Result;
----------------
nit: assert the Tokens is sorted?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:452
+  const HighlightingToken *Last = nullptr;
+  for (const HighlightingToken &Tok : Tokens) {
+    Result.emplace_back();
----------------
note that we don't calculate the column offset for `InactiveCode` token (`{(line, 0), (line, 0)}`), I think we probably calculate the range with the new implementation, maybe add a FIXME.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:489
+    return "member";
+  case HighlightingKind::StaticMethod:
+    return "function";
----------------
It seems ok now, but I think for static method, we should set the modifier to `static`, maybe add a FIXME to support token modifiers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76663