(cpp) std::string* not highlighted in 10.7.1

[milutin]

Describe the issue
There is highlighted regression in 10.7.1 with following line
std::string* p = nullptr;

std::string* p = nullptr;

Which language seems to have the issue?
cpp

Are you using highlight or highlightAuto?
highlightBlock

Sample Code to Reproduce
highlight10.7.1: http://jsfiddle.net/L10xf382/8/
highlighting10.6: http://jsfiddle.net/L10xf382/10/

Expected behavior
highlighting10.6: http://jsfiddle.net/L10xf382/10/

Additional context

[joshgoebel]

See #3005.

The built-in list was removed in favor of a something( rule (using the previous built-in list for hinting only)... it seems some things on that list were not functions though (like std). Could you pease review the list and see if anything else is NOT a function?

-    built_in: 'std string wstring cin cout cerr clog stdin stdout stderr string
stream istringstream ostringstream ' +
-      'auto_ptr deque list queue stack vector map set pair bitset multiset mult
imap unordered_set ' +
-      'unordered_map unordered_multiset unordered_multimap priority_queue make_
pair array shared_ptr abort terminate abs acos ' +
-      'asin atan2 atan calloc ceil cosh cos exit exp fabs floor fmod fprintf fp
uts free frexp ' +
-      'fscanf future isalnum isalpha iscntrl isdigit isgraph islower isprint is
punct isspace isupper ' +
-      'isxdigit tolower toupper labs ldexp log10 log malloc realloc memchr memc
mp memcpy memset modf pow ' +
-      'printf putchar puts scanf sinh sin snprintf sprintf sqrt sscanf strcat s
trchr strcmp ' +
-      'strcpy strcspn strlen strncat strncmp strncpy strpbrk strrchr strspn str
str tanh tan ' +
-      'vfprintf vprintf vsprintf endl initializer_list unique_ptr _Bool complex
 _Complex imaginary _Imaginary',

I see some types in there as well now that I'm looking... CC @klmr

[joshgoebel]

Do all the types from C automatically apply here as well?

[milutin]

I am sorry, I am not C/C++ developer. This regression was found by screenshot testing when I was integrating 10.7.1 to Gerrit.

[joshgoebel]

Ah, ok, we'll wait and see if klmr can chime in then. :) I could take a guess and get MOST of it I think, but a few I'm not sure about.

[klmr]

Yup, that’s intentional. Highlighting of string was inconsistent: C++ has many hundreds of built-in identifiers (functions, types, namespaces and constants), and the previous version of the highligher arbitrarily highlighted just a small handful.

Furthermore, these names can (and frequently are) overridden in custom code. That is, the name string isn’t special in C++, and only a proper parser knows whether it refers to the name from the standard library or another name.

In fact, even the following is legal (and not that uncommon) C++ code:

char* string = "x";

… and here string is a variable name, not a (built-in or otherwise) type, and should definitely not be highlighted as such.

(The same is true to a lesser extent for std: like string, this name can be redefined in custom code except in very specific contexts, and therefore std::x might refer to the non-built-in name.)

tl;dr: it’s an enhancement, not a regression.

[joshgoebel]

Well now that took a turn. Lol. I wouldn't say it was intentional but we can certainly discuss it now that it's come up. Perhaps that other thread did get into this a bit, but I've forgotten now.

Still we need to review the list either way as if there are types hidden there they need to be pulled out and highlighted as types OR else removed entirely as klmr is suggesting.

What is happening right now is by accident though not design.

Aren't string and std pretty common in Cpp code or am I mistaken on that?

[klmr]

Aren't string and std pretty common in Cpp code or am I mistaken on that?

Kind of, but I don’t think the intent of the highlighter is to treat common things specially, is it? Otherwise this might require changing the approach.

As identifiers, neither string nor std have special status in the language per se.

Speaking as a user of syntax highlighting, I feel that consistent highlighting is much more important than highlighting some standard library names but not all, even if the highlighted names are more commonly used. I’d find it more misleading than helpful, especially if there are false positives (i.e. highlighting a name such as string in a context where it doesn’t refer to the built-in name, as in my example above).

Now when it comes to detection, matters are different: string is probably still badly suited since it’s so generic, but presence of std and (exponentially more so!) std:: are a powerful indicator of C++ code.

[joshgoebel]

Kind of, but I don’t think the intent of the highlighter is to treat common things specially, is it?

Perhaps I should have said "convention" vs "common". :) I think we care less about absolute reality vs perceived reality. If you polled 100 C++ uses about whether xyz was a keyword and 90% said yes, then perhaps that's a good argument for highlighting it. :) Perhaps not. I'm fine with going more strict on this for a bit and seeing how many support issues are opened though. I notice GitHub doesn't give either any special treatment.

I feel that consistent highlighting is much more important than highlighting some standard library names but not all, even if the highlighted names are more commonly used

It's a bit hard to argue with this personally, though I wonder if all our users would agree... though I also think we probably do this quite commonly in other grammars at the moment.

...when it comes to detection ... string is probably still badly suited since it’s so generic, but presence of std ... a powerful indicator of C++ code.

This sounds right, but we have to be very careful with auto-detect. If string is often used in C code (because it's a standard library) - AND it' a common keyword/type in many languages, then not giving it relevance in C would bias the engine against C code using strings (since many other languages WOULD claim those keywords for relevancy, but not C). Though this could perhaps be countered by limiting the influence of any particular keyword - something we do not currently do - but likely should. So that's probably what would happen since keeping track of these inter-dependencies across languages quickly gives one a headache. :)

Actually I wonder if this last item has biased keywords in the past to be a bit more "open" vs strict...

[joshgoebel]

@milutin I'm going with @klmr on this one right now. The previous behavior was broken and the new behavior is therefor a fix, not a regression.