Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add documentation to semantic token config #85

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

daiyousei-qz
Copy link
Contributor

This is documentation for configuration added by https://reviews.llvm.org/D148489.

From a maintainability point of view, I think the complete list of kinds/modifiers should be added to "features.md". What do you think? @HighCommander4

Copy link
Contributor

@HighCommander4 HighCommander4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

config.md Outdated Show resolved Hide resolved
config.md Outdated Show resolved Hide resolved
config.md Outdated Show resolved Hide resolved
config.md Outdated Show resolved Hide resolved
@HighCommander4
Copy link
Contributor

HighCommander4 commented May 17, 2023

From a maintainability point of view, I think the complete list of kinds/modifiers should be added to "features.md". What do you think? @HighCommander4

Agree, that would be nice! And, especially for kinds/modifiers which are not part of the standard ones listed in LSP, a short description would also be very useful.

@HighCommander4
Copy link
Contributor

@daiyousei-qz A friendly reminder about this PR. LLVM 17 (the version in which the new configs will ship) has branched and will be released in about 5-6 weeks.

@daiyousei-qz
Copy link
Contributor Author

Sorry for the long absence. I've updated features.md and added a section of semantic highlighting, including a few tables discussing available kinds/modifiers. The available version was read from the git submit date of the enum and I picked a release that's at least a month away. I don't check with different versions of clangd. As the pick is conservative, some items may be available one version earlier.

The deployment of the website in README isn't very friendly to Windows user. Could you please help me ensure that table is well formatted in the website? Thanks!

Copy link
Contributor

@HighCommander4 HighCommander4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

config.md Outdated Show resolved Hide resolved
config.md Outdated Show resolved Hide resolved
features.md Outdated

| Kind | LSP Name | Note
|------------------------|---------------|--------------------------
| Variable{.v9} | variable |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need separate columns for clangd's internal kind and the LSP kind. The internal kind is not exposed to the client in any way.

Let's just combine them into one column, we can call it LSP Kind. (And e.g. variable only needs one row.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clangd's internal kinds are needed for https://reviews.llvm.org/D148489, where we use that as configuration items.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point! When reviewing https://reviews.llvm.org/D148489, I didn't fully realize the implication that we were exposing the internal kinds via this config mechanism. In retrospect, it would probably have been better to have the config keys be the LSP names, but it's too late for that now. (We can still consider a future change to make the internal kinds be 1:1 with the LSP kinds.)

features.md Outdated Show resolved Hide resolved
features.md Outdated Show resolved Hide resolved
features.md Outdated

| Modifier | LSP Name | Note
|----------------------|----------------|-----------
| Declaration{.v12} | declaration |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise here, we don't need a separate column for the internal modifier.

Copy link
Contributor

@HighCommander4 HighCommander4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Please rebase the patch onto the main branch.

features.md Outdated Show resolved Hide resolved

clangd supports following standard LSP token kinds:

| Kind | LSP Name | Since Version | Note
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add an explanation here, like:

Kind is the name recognized in the config file, LSP Name is the name sent to the client.

(And a similar line for modifiers)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the explanation to the config page.

@daiyousei-qz
Copy link
Contributor Author

@HighCommander4 Do you want me to also squash the commits into one?

features.md Outdated Show resolved Hide resolved
@HighCommander4 HighCommander4 merged commit 5b27090 into llvm:main Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants