Emit publishSemanticHighlighting in LSP if enabled
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 34054 Build 34053: arc lint + arc unit
Event Timeline
clang-tools-extra/clangd/SemanticHighlighting.h | ||
---|---|---|
32 | The structures defined here are similar to the structures proposed in LSP SemanticHighlightingParams and SemanticHighlightingInformation, we could define these structures in the Protocol.h (you can find similar pattern there). |
clang-tools-extra/clangd/ClangdLSPServer.cpp | ||
---|---|---|
332 | 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). | |
clang-tools-extra/clangd/Protocol.h | ||
1179 | 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. | |
1182 | nit: using int is fine here. | |
1189 | Add a comment: /// Parameters for the semantic highlighting (server-side) push notification. | |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
120 | add unittests for this function? | |
132 | the implementation requires Tokens sorted, maybe consider using a map to the group-by? | |
152 | 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. | |
clang-tools-extra/clangd/SemanticHighlighting.h | ||
8 | 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). | |
19 | as this kind is used as the lookup table index, let's add Variable = 0 | |
37 | I'd name it textMateScopeLookupTable | |
40 | 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). | |
46 | just toSemanticHighlightingInformation. |
clang-tools-extra/clangd/ClangdLSPServer.cpp | ||
---|---|---|
333 | I'd put the logic here rather than in ClangdServer.cpp, just ClangdServerOpts.SemanticHighlighting = ClangdServerOpts.HiddenFeatures && Params.capabilities.SemanticHighlighting;. | |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
131 | here we lookup the map twice, we could save one cost of lookup. TokenLines[LindIdx].push_back(Token) should work. | |
143 | could we have some comments explaining the details about encoding Token here? | |
144 | if the token is across multiple line, we will emit an ill-formed results. | |
clang-tools-extra/clangd/SemanticHighlighting.h | ||
39 | just: convert to LSP's semantic highlighting information. | |
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp | ||
103 ↗ | (On Diff #207241) | we could drop the explicit HighlightingToken, just ... Tokens = { {HighlightingKind::Variable, Range {...}}, {....} } |
104 ↗ | (On Diff #207241) | Range{ /*start*/{3, 8}, /*end*/{3, 12} } should be compilable. |
112 ↗ | (On Diff #207241) | here as well, just { {1, "...." } }. |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
144 | There's a FIXME above (which is where it should probably be handled). A bit unsure how to solve though. If a token is a block comment spanning multiple lines we would need to know the length of every line in the block comment. Probably something that can be solved with the ASTContext or SourceManager but that can't be accessed in this function. | |
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp | ||
104 ↗ | (On Diff #207241) | Doesn't compile because of the default initialization of line and character in Position. |
clang-tools-extra/clangd/ClangdLSPServer.cpp | ||
---|---|---|
409 | The proposal says If the client declares its capabilities with respect to the semantic highlighting feature, and if the server supports this feature too, the server should set all the available TextMate scopes as a "lookup table" during the initialize request.. now it seems that clangd always emit this lookup table to the client. | |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
70 | The name U doesn't provide much information, maybe Bytes? | |
134 | 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). | |
137 | don't repeat what the code does in the comment. Here we are encoding a Token in a form specified in the proposal, I think we should have high-level comment. | |
144 | oh, i missed that FIXME, the FIXME is a bit far away, maybe move it here (now we assume the token is always on the same line). | |
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp | ||
109 ↗ | (On Diff #207563) | nit: ActualResults? |
111 ↗ | (On Diff #207563) | nit: ExpectedResults? |
114 ↗ | (On Diff #207563) | nit: we should use EXPECT_* when comparing the expected results and actual results. |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
---|---|---|
134 | They aren't "fully" encoded yet though, they get encoded after the inner loop is done. How about LineByteTokens? | |
144 | I think that this should probably be handled above (so if a token covers N different lines it would be separated into N tokens in the TokenLines map. | |
clang-tools-extra/clangd/SemanticHighlighting.h | ||
8 | Changed the initialize-params test to have semantic highlighting enabled as well. |
mostly good, a few more nits.
clang-tools-extra/clangd/Protocol.h | ||
---|---|---|
1187 | nit: remove the blank line to be consistent with the existing style in this file. | |
1199 | nit: remove this blank line. | |
clang-tools-extra/clangd/SemanticHighlighting.cpp | ||
121 | nit: remove the "{}" | |
134 | sounds good. | |
138 | add |<--- 4 bytes --->|<-2 bytes->|<-2 bytes->| | character | length | index | to the comment, it helps reader to understand how the encode like. | |
156 | nit: remove the {} for one-body statement. | |
clang-tools-extra/clangd/SemanticHighlighting.h | ||
7 | 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 .... // //===---------------------------------------------------------------------------------===// | |
clang-tools-extra/clangd/test/initialize-params.test | ||
3 | How about moving this to the new semantic-highlighting.test? |
thanks, looks good.
clang-tools-extra/clangd/ClangdLSPServer.h | ||
---|---|---|
59 | nit: clang-format. | |
clang-tools-extra/clangd/test/semantic-highlighting.test | ||
37 | nit: we could just verify this field like: # CHECK: "id": 0, # CHECK: " "semanticHighlighting": { # CHECK-NEXT: "scopes": [ # CHECK-NEXT: [ # CHECK-NEXT: "variable.cpp" # CHECK-NEXT: ], # CHECK-NEXT: [ # CHECK-NEXT: "entity.name.function.cpp" # CHECK-NEXT: ] # CHECK-NEXT: ] # CHECK-NEXT: }, |
nit: clang-format.