Page MenuHomePhabricator

[clangd] Added the vscode SemanticHighlighting feature code but did not enable it in the client.
ClosedPublic

Authored by jvikstrom on Aug 9 2019, 1:37 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jvikstrom created this revision.Aug 9 2019, 1:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 1:37 AM
jvikstrom updated this revision to Diff 214334.Aug 9 2019, 1:39 AM

Added comment to decodeTokens function.

Harbormaster completed remote builds in B36511: Diff 214334.
hokein added inline comments.Aug 9 2019, 4:52 AM
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
37 ↗(On Diff #214334)

nit: name it SemanticHighlightingFeature.

38 ↗(On Diff #214334)

I believe the type of lookup table returned from clangd is a string[][]. I'd name it scopeLookupTable

62 ↗(On Diff #214334)

I think the params here is defined as SemanticHighlightingParams from the LSP proposal, could we use the names as defined in the LSP for these structures (e.g. SemanticHighlightingParams, SemanticHighlightingInformation)?

jvikstrom updated this revision to Diff 214364.Aug 9 2019, 5:35 AM
jvikstrom marked 4 inline comments as done.

Address comments.

clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
62 ↗(On Diff #214334)

Renaming HighlightingToken to SemanticHighlightingToken to be consistent with those names as well.

nridge added a subscriber: nridge.Aug 10 2019, 12:14 PM
hokein added inline comments.Aug 12 2019, 5:10 AM
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
8 ↗(On Diff #214364)

sorry for not being clear in the previous comment, I think we should mirror definitions from the LSP proposal, the same to the SemanticHighlightingInformation.

You may need to add a vscode-languageserver-types dependency.

32 ↗(On Diff #214364)

I think here we should use SemanticHighlightingParams in the generic type parameter list

38 ↗(On Diff #214364)

could you add a comment on this?

63 ↗(On Diff #214364)

this seems doesn't do anything, can we defer it to a follow-up patch?

jvikstrom updated this revision to Diff 214613.Aug 12 2019, 5:51 AM
jvikstrom marked 4 inline comments as done.

Mirror the LSP proposal SemanticHighlightingParams/Information types.

hokein accepted this revision.Aug 12 2019, 6:17 AM

mostly good with a few nits.

clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
9 ↗(On Diff #214613)

could you add a comment describing this is a mirror of LSP definition?

16 ↗(On Diff #214613)

nit: I'd remove this blank line to make SemanticHighlightingParams and SemanticHighlightingInformation group closer as they are mirrors of LSP definitions.

27 ↗(On Diff #214613)

the comment seems not precise to me, this is the data decoded from the base64-encoded string sent by clangd.

33 ↗(On Diff #214613)

nit: scope lookup table.

34 ↗(On Diff #214613)

nit: maybe name it scopeIndex.

This revision is now accepted and ready to land.Aug 12 2019, 6:17 AM
This revision was automatically updated to reflect the committed changes.
jvikstrom marked 5 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2019, 6:34 AM