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