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

Enable semantic highlighting support for the C++ language client #5850

Merged
merged 1 commit into from
Aug 23, 2019

Conversation

HighCommander4
Copy link
Contributor

Signed-off-by: Nathan Ridge zeratul976@hotmail.com

The C++ language server, clangd, now has basic semantic highlighting support. This patch allows Theia to make use of it by enabling the semantic highlighting feature in the C++ language client.

cc @jvikstrom

@akosyakov akosyakov added the cpp issues related to the C/C++ language label Aug 5, 2019
@jvikstrom
Copy link

Hello,

Just thought I'd chime in a bit as I've been developing/testing this feature primarily using theia. These are the same changes I did to theia locally to enable semantic highlighting and it seems to work well. So the changes look good to me.

There is one issue though, but I think it is related to the underlying client implementation in Theia. I haven't looked into the Theia codebase to see what is causing it. But from what I can tell, clangd sends the correct source ranges for the test cases.

This is the issue that I created for it.

(Thanks for tagging me by the way)

@hokein
Copy link

hokein commented Aug 5, 2019

@HighCommander4 thanks for doing it.

just a note from clangd, this feature is still under development and may have bugs. In the coming-release clangd-9, it is considered experimental (clangd only emits highlightings for basic symbols).

The PR here will enable the semantic highlighting in clangd by default (not sure whether theia needs an experimental flag for it).

@simark
Copy link
Contributor

simark commented Aug 5, 2019

Can you give an example of what constructs are currently highlighted by clangd? Bonus points if they are thing not correctly handled by our textmate grammars.

@jvikstrom
Copy link

Can you give an example of what constructs are currently highlighted by clangd? Bonus points if they are thing not correctly handled by our textmate grammars.

Currently clangd is highlighting these kinds of symbols:

  • variables (all the different kinds (globals, parameters, etc..) as the same textmate type, except for members)
  • functions
  • methods
  • member variables
  • class "names"
  • enums
  • enum constants
  • namespaces
  • template parameters

(things that are typedefed will also be highlighted as their underlying type)

So for example in this case:

// Somewhere in a header
namespace aaa {
  struct A {
    static int a;
  }
}
// The current source file
#include ...
void f() {
  aaa::A::a = 123;
}

Clangd will highlight aaa as a namespace, A as a class and a as a variable (as is, the default theia theme will give aaa and A the same color though).

I guess another thing that might be incorrectly handled by TM grammars are enums/scoped enums and things like that.

@HighCommander4
Copy link
Contributor Author

Template parameters would be another case that clangd highlights but the TM grammar does not.

@HighCommander4
Copy link
Contributor Author

@simark do you think we can proceed with this?

@paul-marechal
Copy link
Member

@HighCommander4 I would like to try your patch, can you tell me what to look for exactly?

I am afraid I would not see what is being highlighted by clangd and what is from the TM grammars :)

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

image

(clangd highlighting on the left, regular TextMate on the right)

Tried to compare with master, this is much better indeed!

Signed-off-by: Nathan Ridge <zeratul976@hotmail.com>
@HighCommander4
Copy link
Contributor Author

(Updated to resolve conflicts)

@HighCommander4
Copy link
Contributor Author

HighCommander4 commented Aug 21, 2019

Tried to compare with master, this is much better indeed!

It will get even better after I upstream some clangd patches, and figure out how to make corresponding customizations in Theia :)

@simark
Copy link
Contributor

simark commented Aug 21, 2019

@simark do you think we can proceed with this?

I haven't had the time to try it, but looking at Paul's screenshot, it seems very nice. No objection from me to merge this.

@paul-marechal paul-marechal merged commit ffcb3fa into eclipse-theia:master Aug 23, 2019
@MarkZ3
Copy link
Contributor

MarkZ3 commented Aug 24, 2019

It looks nice! Thanks a lot for working on this.

@HighCommander4
Copy link
Contributor Author

The credit goes to @kittaakos for the LSP proposal and Theia client implementation, and @jvikstrom for the C++ server implementation. I'm just enabling it, and polishing things here and there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp issues related to the C/C++ language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants