Page MenuHomePhabricator

[clangd] Add distinct highlightings for declarations of functions and methods
Needs ReviewPublic

Authored by nridge on Aug 29 2019, 9:41 PM.

Event Timeline

nridge created this revision.Aug 29 2019, 9:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2019, 9:41 PM

Should we have different highlightings for declarations vs usages? (Although I guess in the end it will be up to the editor if they highlight them differently as the scope is just more specific for declarations)
I guess I personally don't really see the reason as it should be clear from the context if it's a declaration or a function call.
But this might actually be nice if you have macros that declare functions

Should we have different highlightings for declarations vs usages? (Although I guess in the end it will be up to the editor if they highlight them differently as the scope is just more specific for declarations)
I guess I personally don't really see the reason as it should be clear from the context if it's a declaration or a function call.

I also second the opinion that usages and declarations should be highlighted the same.
Both represent the same thing, therefore they should have the same color.

And this highlighting does not seem to add any extra information too - it's obvious from the code whether a name is a declaration or a usage.
Even in the macro cases, the users usually know the semantics of the macro or can infer from the name.

There is precedent for highlighting declarations and uses differently in other C++ editors. For example, Eclipse CDT has separate highlightings for function and functions declarations (see screenshot below).

Objectively speaking, I think it makes sense to style function declarations differently from function uses for emphasis. For example, you can use the same color for both, but make the declarations bold. I have found this to aid readability.

There is precedent for highlighting declarations and uses differently in other C++ editors. For example, Eclipse CDT has separate highlightings for function and functions declarations (see screenshot below).

Acknowledged.

Objectively speaking, I think it makes sense to style function declarations differently from function uses for emphasis. For example, you can use the same color for both, but make the declarations bold. I have found this to aid readability.

As mentioned before, I think doing this:

  • adds some confusion: the same thing highlighted in two different colors,
  • provides almost zero value: function declarations have completely different syntax from function usages anyway, there are more than enough signals to distinguish them and we don't need semantic highlighting for that.

I think it makes sense to style function declarations differently from function uses for emphasis. For example, you can use the same color for both, but make the declarations bold. I have found this to aid readability.

I think this makes sense, but it should be done for all declarations and not just functions.
But doing that in the current design would mean we need to double the number of styles. In addition to field, method, function, we would need field-declaration, method-declaration, function-declaration, etc.
Combinatorial explosion of that kind is bad, so I would rather avoid it.

That would also mean we would rely on every editor to apply consistent styling for this, at least by default: make somethind-decl just like decl but bold.
I don't think there's any way to enforce this.

If there was an alternative design we could use for this that would not cause combinatorial explosion and would allow us to consistently apply the same modifier to different kinds of highlighting (e.g. something that will consistently turn to bold in all editors that support this), I would be supportive of implementing this.

nridge added a comment.Sep 2 2019, 2:47 PM

I think this makes sense, but it should be done for all declarations and not just functions.
But doing that in the current design would mean we need to double the number of styles. In addition to field, method, function, we would need field-declaration, method-declaration, function-declaration, etc.
Combinatorial explosion of that kind is bad, so I would rather avoid it.

I agree that a combinatorial explosion would be unfortunate. I actually think that having a linear list of semantic highlighting kinds is a weakness of this protocol extension for exactly this reason.

I think the way cquery does it it better in this regard: in place of a single kind enum, they essentially have a 4-tuple of (kind, parent kind, storage class, role). With this setup:

  • The kinds can be a relatively small set of fundamental language entity kinds (variable, function, class, namespace, etc.)
  • Things like "field" can be expressed as kind = variable, parentKind = class
  • The storage class allows differentiating things like static vs. nonstatic without having to introduce distinct kinds for those.
  • The role allows differentiating declarations from uses, and even fancier things like read occurrences from write occurrences.

I've been meaning to comment on the upstream protocol proposal to suggest a more flexible approach like this, though I'm not sure how it would integrate with TextMate scopes (which cquery doesn't bother with).

I was hoping though that a patch like this, which would bring us largely to parity with Eclipse CDT's highlightings, wouldn't need to blocked on a change to the upstream protocol proposal, which could take a while.

That would also mean we would rely on every editor to apply consistent styling for this, at least by default: make somethind-decl just like decl but bold.
I don't think there's any way to enforce this.

Keep in mind that since the scopes for declarations just add .declaration to the scopes for the base entities, by default they would be highlighted the same as the base entity. Only if a user or theme goes to the trouble of adding a specific rule for the .declaration, would they look different, and in that case I think we can trust the judgment of the user or theme author.

I was hoping though that a patch like this, which would bring us largely to parity with Eclipse CDT's highlightings, wouldn't need to blocked on a change to the upstream protocol proposal, which could take a while.

Is feature parity a hard requirement? Having slightly different highlightings in that case should not be too disruptive.

I think the way cquery does it it better in this regard: in place of a single kind enum, they essentially have a 4-tuple of (kind, parent kind, storage class, role).

A design like this definitely makes more sense. I was thinking of a slightly simpler model: adding a set of "modifiers" to each highlighting should be enough to encode all of it, e.g. a modifier 'is class member' could be used to distinguish methods and fields from global functions and variables, a modifier 'is usage' can be used to distinguish usages from declarations, etc.
But there's no combinatorial explosion in the cquery's model, which is the important bit.

That would also mean we would rely on every editor to apply consistent styling for this, at least by default: make somethind-decl just like decl but bold.
I don't think there's any way to enforce this.

Different designs lead to different interpretations: if we encode this as a separate highlighting kind, there's a greater chance this would be highlighted in different color rather than just applying modifiers.
If we model this as "something in addition to the main highlighting", there's a greater chance this would be interpreted as something that should be "bold".

Agree there is no way to enforce this.

I was hoping though that a patch like this, which would bring us largely to parity with Eclipse CDT's highlightings, wouldn't need to blocked on a change to the upstream protocol proposal, which could take a while.

Is feature parity a hard requirement? Having slightly different highlightings in that case should not be too disruptive.

Not a hard requirement, just a nice-to-have for someone moving from one tool to another :)

If you feel that for now it's better not to do this, I can respect that.

I think the way cquery does it it better in this regard: in place of a single kind enum, they essentially have a 4-tuple of (kind, parent kind, storage class, role).

A design like this definitely makes more sense. I was thinking of a slightly simpler model: adding a set of "modifiers" to each highlighting should be enough to encode all of it, e.g. a modifier 'is class member' could be used to distinguish methods and fields from global functions and variables, a modifier 'is usage' can be used to distinguish usages from declarations, etc.
But there's no combinatorial explosion in the cquery's model, which is the important bit.

I will suggest this for the upstream protocol and see where that goes.

Not a hard requirement, just a nice-to-have for someone moving from one tool to another :)
If you feel that for now it's better not to do this, I can respect that.

Thanks, if that works for you, I would wait until we get/build a better support in the protocol for this.
If Eclipse CDT gets a lot of user complaints and this turns out to be an important missing feature, happy to reconsider.
FWIW, I find having Eclipse CDT on board to be important for us and therefore happy to make trade-offs when needed.
This is a small thing, though, so waiting for an explicit support for this in the protocol does not sound too bad.

I will suggest this for the upstream protocol and see where that goes.

SG, keep us posted!

I think the way cquery does it it better in this regard: in place of a single kind enum, they essentially have a 4-tuple of (kind, parent kind, storage class, role).

A design like this definitely makes more sense. I was thinking of a slightly simpler model: adding a set of "modifiers" to each highlighting should be enough to encode all of it, e.g. a modifier 'is class member' could be used to distinguish methods and fields from global functions and variables, a modifier 'is usage' can be used to distinguish usages from declarations, etc.
But there's no combinatorial explosion in the cquery's model, which is the important bit.

I will suggest this for the upstream protocol and see where that goes.

For reference, I made this suggestion here.