Page MenuHomePhabricator

[clangd] Added highlighting for non type templates.
ClosedPublic

Authored by jvikstrom on Aug 14 2019, 8:20 AM.

Details

Summary

Highlights pointers to variables as variables. Function pointers are highlighted as functions. Member function pointers are highlighted as members. Values are highlighted as TemplateParameter.

Diff Detail

Repository
rL LLVM

Event Timeline

jvikstrom created this revision.Aug 14 2019, 8:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2019, 8:20 AM
ilya-biryukov added inline comments.Aug 14 2019, 9:26 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
231 ↗(On Diff #215129)

Why do we special-case template parameters, but not other kinds of variables?
We definitely need a comment explaining why template parameters are handled in a special way, but variables, parameters, fields are not.

jvikstrom marked an inline comment as done.Aug 16 2019, 12:55 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/SemanticHighlighting.cpp
231 ↗(On Diff #215129)

Not quite sure what you mean about variables/parameters/fields not being handled in a special way.

The reason for special casing non type templates is because it probably gives more information/is more valuable to highlight a reference/pointer as a variable rather than a normal template parameter (same for methods/functions).

But maybe they all should just be highlighted as with the TemplateParameter kind instead?

ilya-biryukov added inline comments.Aug 16 2019, 1:47 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
231 ↗(On Diff #215129)

Not quite sure what you mean about variables/parameters/fields not being handled in a special way.

Non-type template parameters are very similar to global and local variables, function parameters, class fields, etc.
We could also match those on type and highlight differently based on the type.

The reason for special casing non type templates is because it probably gives more information/is more valuable to highlight a reference/pointer as a variable rather than a normal template parameter (same for methods/functions).

However, if a global variable has a function pointer type we do not highlight it as a function. Why would this be different?

But maybe they all should just be highlighted as with the TemplateParameter kind instead?

I would personally vouch for this option. The highlighting functionality lets me understand what the name resolves to; if it's a template parameter and it would be highlighted as a variable instead, this would create confusion on my end. If I need to know the type I'll look at completion results or hover.

It is my personal preference and I'm ok with this going in a different direction if you feel the opposite is better. Just asking to put the rationale of this decision (why do this only for template parameters and not other things) in a comment somewhere in the source code.

jvikstrom updated this revision to Diff 215544.Aug 16 2019, 2:16 AM
jvikstrom marked an inline comment as done.

Highlight as TemplateParameter.

clang-tools-extra/clangd/SemanticHighlighting.cpp
231 ↗(On Diff #215129)

I would personally vouch for this option. The highlighting functionality lets me understand what the name resolves to;

Very true actually, was a bit unsure what was the correct way to go but this is very true actually.

Changing to just highlight as TemplateParameter.

This revision is now accepted and ready to land.Aug 16 2019, 2:22 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2019, 2:29 AM