Skip to content

Conversation

icecream17
Copy link
Contributor

@icecream17 icecream17 commented Feb 2, 2023

Description of change:

incorporates:

Alternate Designs

Note for the first incorporation, I only syntax color the last function name and not the namespace or ::. But that could easily be changed.

For future me, the relevant tree-sitter namespace identifier scopes are:
call_expression > qualified_identifier > identifier and
function_declarator > qualified_identifier > identifier

I don't know anything about template functions so I left that untouched. So this is probably an incomplete fix.

Finally, add the static_assert operator. It's technically a directive so it'll appear purple, not blue. Again this could easily be changed so don't hesitate about feedback, idk anything about c++

Verification Process

Will download and check to make sure it works

Applicable issues

Relevant to atom/language-c#246, atom/language-c#245 (pretty much a fix except drawback),

(I split these out bc github doesn't detect that "fixes" applies to multiple issues)
Fixes atom/language-c#169
Fixes atom/language-c#170
Fixes atom/language-c#260
Fixes microsoft/vscode-cpptools#1477

For tree-sitter, kinda fix the Discord reported issue (In #support M1 Mac C++ Syntax highlighting)

incorporates:

atom/language-c#252
Note that this particular change was modified. `class public virtual : public virtual Example` is invalid I think, but I don't see a good way to prevent that because the detection must be moved into `patterns > include` because textmate is not multiline regex.
See also jeff-hykin/better-cpp-syntax#14
(In fact that whole repostory probably has some improvements)
I changed it to include the angle brackets because types can have those

atom/language-c#263
atom/language-c#311
atom/language-c#368

For tree-sitter, kinda fix the Discord reported issue
(In #support M1 Mac C++ Syntax highlighting)

Note that I syntax color only the last function name and not the namespace or colon. But that could easily be changed.

For future me, the relevant tree-sitter namespace identifier scopes are:
`call_expression > qualified_identifier > identifier` and
`function_declarator > qualified_identifier > identifier`

I don't know anything about template functions so I left that untouched. So this is probably an incomplete fix.

Finally, add the `static_assert` operator. It's technically a directive so it'll appear purple, not blue. Again this could easily be changed so don't hesitate about feedback, idk anything about c++
@confused-Techie
Copy link
Member

This looks like a fantastic commit and is very much appreciated! Some research will have to be done to see exactly how this behaves, but ideally @mauricioszabo or @pulsar-edit/core could provide some insight into how accurate this all looks.

But otherwise thanks a ton for your contribution!

@mauricioszabo
Copy link
Contributor

@icecream17 do you mind including a code that this fix applies to?

@icecream17
Copy link
Contributor Author

icecream17 commented Feb 7, 2023

// class declaration and inheritance on multiple lines (non-tree-sitter)
class A :
    public B,
    public C,
    public D<GenericType>
{
    // static_assert in both non-tree-sitter and tree-sitter
    static_assert(true);
    public: void print();
};

// compare with
class E : public F, public G, public H<GenericType> {}

// virtual public vs public virtual (both should work)
class I : virtual public J {};
class K : public virtual L {};

// "namespaced" function (idk what this is called)
void A::print()
{
    // (tree-sitter) `<` and `>` are now properly selected
    bool unused = ( 1 < 2 ) && ( 4 > 3 );
}

// "nested namespace definition" c++17 (see https://en.cppreference.com/w/cpp/language/namespace#Namespaces)
namespace abc::def::ghi {}

Note that github uses textmate so some of these don't syntax highlight in github too

@mauricioszabo
Copy link
Contributor

Just for the record, and because I quite like these things: here's the code highlighted without the fix:
image

And here is being highlighted with the fix:
image

I made a fix (missing ; on line 13, otherwise class would not be highlighted) but yep, it works better

@DeeDeeG
Copy link
Member

DeeDeeG commented Feb 9, 2023

Good to see modernization of the language support!

And glad to see @mauricioszabo's approval, and the CI results are within current expectations.

I'm going to go ahead and merge this one!

Thanks for this @icecream17!

@DeeDeeG DeeDeeG merged commit f3742aa into pulsar-edit:master Feb 9, 2023
@DeeDeeG
Copy link
Member

DeeDeeG commented Feb 9, 2023

Oh, yeah... If anyone would be interested in writing test cases for this, it would be 5,000 and a half % awesome.

Didn't want to require it for merging, and yet, it would make a fantastic follow-up PR to this.

@icecream17 icecream17 deleted the c++fix branch February 9, 2023 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants