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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
