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

enh(cpp) recognize primitive types (int, char, etc.) as keywords #3313

Closed
wants to merge 2 commits into from
Closed

enh(cpp) recognize primitive types (int, char, etc.) as keywords #3313

wants to merge 2 commits into from

Conversation

deniskovalchuk
Copy link
Contributor

@deniskovalchuk deniskovalchuk commented Aug 23, 2021

I would like to discuss the change made in hljs 11.0.0:

As part of the above pull request, primitive types and keywords were split to
be highlighted separately (commit 1be7a28). It seems to me that after the
change, the highlighting of some C++ code looks unnatural and confusing. For
example, take a look at the variable declarations before and after the change:

Before (jsfiddle):
10 7 1

After (jsfiddle):
11 2 0

How would you feel about returning to the previous behaviour?

Technically, as far as the C++ standard is concerned, primitive types are
keywords. Plus, many popular C++ IDEs and text editors highlight primitive
types as keywords:

VisualStudioCode

CLion

XCode

Notepad++

Changes

  • (cpp) Moved primitive types from RESERVED_TYPES to RESERVED_KEYWORDS.
  • (arduino) Used keyword for Arduino specific data types as Arduino is a superset of C++.

Checklist

  • Updated tests
  • Updated the changelog at CHANGES.md

@joshgoebel
Copy link
Member

joshgoebel commented Aug 24, 2021

Technically, as far as the C++ standard is concerned, primitive types are keywords.

This isn't highly relevant though (for us), as often we make decisions that improve the consistency of highlighting (across languages) without overly weighting any particular language. The idea being that highlighting snippets somewhat consistently across languages is a higher (and more achievable) goal than absolutely correctness with accordance to one particular language. IE: It doesn't matter if type is literally a type in C while literally a keyword in CPP, etc... C and C++ code (two different grammar types for us) should highlight similarly - with things that appear to be primitive types highlighted at type.

I think if anything we should perhaps add the type modifiers so that they also highlight as type... making the entire "type definition" (sorry, terminology) highlight as a type... since that seems to be your real concern here - the modifiers having different highlighting than the types themselves.

At the very least...

  • signed int
  • unsigned int

...do seem broken the way we're doing it now, as my understanding is the type name includes the signed/unsigned prefix.

Also worth pointing out this is entirely within the users control via CSS:

.language-cpp .hljs-type {
  color: /* whatever */
}

... if they want to erase the type/keyword distinction for cpp.

@deniskovalchuk
Copy link
Contributor Author

@joshgoebel, thanks for the detailed reply!

Technically, as far as the C++ standard is concerned, primitive types are keywords.

This isn't highly relevant though (for us), as often we make decisions that improve the consistency of highlighting (across languages) without overly weighting any particular language. The idea being that highlighting snippets somewhat consistently across languages is a higher (and more achievable) goal than absolutely correctness with accordance to one particular language. IE: It doesn't matter if type is literally a type in C while literally a keyword in CPP, etc... C and C++ code (two different grammar types for us) should highlight similarly - with things that appear to be primitive types highlighted at type.

I get the point. I just wanted to point out that the current highlighting looks
unusual in the C/C++ context. One small comment: primitive types are also
keywords in C standard :)

I think if anything we should perhaps add the type modifiers so that they also highlight as type... making the entire "type definition" (sorry, terminology) highlight as a type... since that seems to be your real concern here - the modifiers having different highlighting than the types themselves.

At the very least...

  • signed int
  • unsigned int

...do seem broken the way we're doing it now, as my understanding is the type name includes the signed/unsigned prefix.

Okey.

Also worth pointing out this is entirely within the users control via CSS:

.language-cpp .hljs-type {
  color: /* whatever */
}

... if they want to erase the type/keyword distinction for cpp.

The CSS rule also affects type names with the _t suffix, which are usually
highlighted separately from primitive types:

Highlightjs (jsfiddle):
hljs
IDEs:
VisualStudioCode
CLion
XCode

The separation of primitive types and types that have definitions can be
another argument in favor of recognizing primitive types as keywords. May I get
your opinion?

joshgoebel added a commit to joshgoebel/highlight.js that referenced this pull request Aug 28, 2021
@joshgoebel
Copy link
Member

joshgoebel commented Aug 28, 2021

The separation of primitive types and types that have definitions can be
another argument in favor of recognizing primitive types as keywords. May I get
your opinion?

I'm not sure we care about this distinction (a type is a type contextually, IMHO), and if we did perhaps a better way to handle it (if it was shown that more people actually care about this) would be to simply expand our scopes... so we have type right now but then we might also have type.primitive and then people could choose style it however they chose. IE, the highlighting engines scopes could be quite particular, but users can decide (via CSS) than they like fuzzier highlighting.

You can also do this at the grammar level (vs CSS) by patching a grammars classNameAliases table... in this case aliasing type back to keyword, though that doesn't 100% solve the non-keyword types. More specific scoping would.

Closing this in favor of #3316 to make the full type declaration consistent as type.

@joshgoebel joshgoebel closed this Aug 28, 2021
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.

2 participants