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

C++20 keywords don't seem to be highlighted #396

Open
2 of 5 tasks
cjdb opened this issue Oct 11, 2019 · 29 comments
Open
2 of 5 tasks

C++20 keywords don't seem to be highlighted #396

cjdb opened this issue Oct 11, 2019 · 29 comments
Labels
duplicate This issue or pull request already exists ✨ Enhancement New feature or request Syntax: C++ issue exists for C++

Comments

@cjdb
Copy link

cjdb commented Oct 11, 2019

Checklist

  • This problem exists even with the setting "C_Cpp.enhancedColorization": "Disabled"
  • This bug exists for C
  • This bug exists for C++
  • This bug exists for Objective-C
  • This bug exists for Objective-C++

The code with a problem is:

module example;
import <vector>
import some.other.module;

export template<typename From, typename To>
concept convertible_to =
	std::is_convertible_v<From, To> and
	requires(From (&f)()) {
		static_cast<To>(f());
	};

In the above example, module, import, <vector>, some.other.module, concept, and requires should all be highlighted, but they're not.

It looks like:

image

Theme: peaceshi

image

Theme: XD

image

Theme: One Monokai

It should look like:

  • module, import, concept, and requires should all be highlighted like the other keywords. (Note: module is contextually-dependent.)
  • <vector> and some.other.module should probably look like <vector> in #include <vector>
@cjdb
Copy link
Author

cjdb commented Oct 11, 2019

I'd be okay with some.other.module not being highlighted.

@matter123
Copy link
Collaborator

Duplicate of #302. Current focus is on published standards.

@matter123 matter123 added duplicate This issue or pull request already exists Syntax: C++ issue exists for C++ ✨ Enhancement New feature or request labels Oct 11, 2019
@matter123
Copy link
Collaborator

Just keyword highlighting, no context awareness, could be added in easily enough. Import as an alternative to #include should also be easy enough to add.

@cjdb
Copy link
Author

cjdb commented Oct 11, 2019

I've been trying to work out how to do this locally, but without success. Looks like the above words are already in the JSON file, which has confused me.

@matter123
Copy link
Collaborator

matter123 commented Oct 11, 2019

Often those are in negative lookbehinds and lookaheads.

import: the import keyword needs to be pulled out of the check for #, But if the semicolon is required it might be best if its pulled into its own pattern.

Adding keywords: This should be just as simple as adding the correct flags to the keywords in https://github.com/jeff-hykin/cpp-textmate-grammar/blob/master/source/languages/cpp/tokens.rb#L261-L274

@cjdb
Copy link
Author

cjdb commented Oct 11, 2019

Thanks. I was definitely barking up the wrong tree, trying to add what's below to generate.rb.

grammar[:concept_definition] = Pattern.new(
        tag_as: "meta.declaration.concept",
        should_fully_match: ["concept A = B;"]
        match: Pattern.new(
                match:/concept/,
                tag_as: "keyword.other.concept",
            ).maybe(@spaces).then(
                identifier
            ).maybe(@spaces).then(
                assignment_operator
            ).maybe(@spaces).then(
                /[^;]+/
            ).maybe(@spaces).then(@semicolon.or(/\n/)),
    )

@matter123
Copy link
Collaborator

matter123 commented Oct 11, 2019

Looks good, couple changes.

  1. use .then(std_space) rather than .maybe(@spaces). This is for a couple reasons:
    • std_space supports inline comments
    • std_space is automatically optional
    • .maybe(@spaces) can catastrophically backtrack.
  2. .then(/[^;]+/) should be .oneOrMoreOf(match: /[^;]/, dont_backtrack?: true)
  3. use @end_of_line instead of /\n/
  4. You still want a fallback to highlight /concept/ if the context aware pattern fails.

@matter123 matter123 reopened this Oct 11, 2019
matter123 added a commit that referenced this issue Oct 11, 2019
import support is now correct
@cjdb
Copy link
Author

cjdb commented Oct 11, 2019

Do concept, requires, and import need logic like what's above? I get module probably does, but the others aren't context-dependent keywords.

@matter123
Copy link
Collaborator

matter123 commented Oct 11, 2019

If a keyword doesn't influence any other highlighting, then all that is need is to add the correct flags inside tokens.rb. In the new PR, I added support for import.

@cjdb
Copy link
Author

cjdb commented Oct 11, 2019

I think this is a rookie mistake, but I'm having issues with npm run build.

cjdb: npm test

> better-syntax@1.0.0 pretest /mnt/c/Users/Chris/projects/cpp-textmate-grammar
> npm run build


> better-syntax@1.0.0 build /mnt/c/Users/Chris/projects/cpp-textmate-grammar
> ruby scripts/generate.rb

scripts/generate.rb:6: warning: Insecure world writable dir /mnt/c/Users/Chris/projects/cpp-textmate-grammar/node_modules/.bin in PATH, mode 040777
Traceback (most recent call last):
        4: from /mnt/c/Users/Chris/projects/cpp-textmate-grammar/source/languages/c/generate.rb:3:in `<main>'
        3: from /mnt/c/Users/Chris/projects/cpp-textmate-grammar/source/languages/c/generate.rb:3:in `require_relative'
        2: from /mnt/c/Users/Chris/projects/cpp-textmate-grammar/source/textmate_tools.rb:4:in `<top (required)>'
        1: from /usr/lib/ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require'
/usr/lib/ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require': cannot load such file -- deep_clone (LoadError)


Generating the syntax for 'c' failed: 256
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! better-syntax@1.0.0 build: `ruby scripts/generate.rb`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the better-syntax@1.0.0 build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/cjdb/.npm/_logs/2019-10-11T04_10_51_315Z-debug.log
npm ERR! Test failed.  See above for more details.

Are there any prerequisites I should be installing? I've run npm install and npm install top to no avail.

@matter123
Copy link
Collaborator

Yes bundle install, I should probably add a post install hook.

matter123 added a commit that referenced this issue Oct 13, 2019
@jeff-hykin
Copy link
Owner

Yes bundle install, I should probably add a post install hook.

I went ahead and added that to the CONTRIBUTING.md for next time. Sorry, about that @cjdb, and thanks for taking the initiative to get things running on your own machine.

@cjdb
Copy link
Author

cjdb commented Oct 18, 2019

I've noticed that since pulling 9f50b88, my custom requires highlighting has stopped working. Highlighting doesn't show even when it's configured identical to consteval, but interestingly, it doesn't highlight at all when I try to use it like a normal function.

@matter123
Copy link
Collaborator

It is expected that requires is not usable as a function name due to, avoid_invalid_function_names = @cpp_tokens.lookBehindToAvoidWordsThat(:isWord, not(:isPreprocessorDirective), not(:isValidFunctionName)).

Can you share what you have for the requires highlighting? As well as what context you added it to.

@cjdb
Copy link
Author

cjdb commented Oct 18, 2019

Can you share what you have for the requires highlighting?

# source/language/cpp/tokens.rb
{ representation: "consteval", name: "consteval", isLambdaSpecifier: true },
{ representation: "requires",  name: "requires",  isLambdaSpecifier: true },

Same as consteval for now. Figured I'd start small, and work my way up.

As well as what context you added it to.

[]() consteval x {}; // consteval highlighted
[]() requires x {}; // requires not highlighted

@matter123
Copy link
Collaborator

If you are using the debugger could you check that you are using the "Launch and Build All" configuration, the default "Launch Extension" does not usually rebuild the syntax files.

@cjdb
Copy link
Author

cjdb commented Oct 18, 2019

I've been manually running npm test, but I just tried Launch and Build All: no difference.

@matter123
Copy link
Collaborator

matter123 commented Oct 18, 2019

That is very strange, If I add the requires token directly after consteval (line 221) everything seems to work.

Screen Shot 2019-10-17 at 18 47 57

Could you remove the other requires definition (on line 263) just to be sure?

Is the lambda definition inside of a macro?

@jeff-hykin Any thoughts on why it wouldn't work?

@cjdb
Copy link
Author

cjdb commented Oct 18, 2019

Still nothing. Is there any chance that the customised file isn't being loaded, and the default one is?
image

@matter123
Copy link
Collaborator

There shouldn't be, npm test has no knowledge of what vscode loads normally, and the extension development host replaces the extension, so that the default one should never be loaded.

@matter123
Copy link
Collaborator

Is the pattern being recognized as a lambda? (Is the scope at { meta.function.definition.body.lambda.cpp when using Developer: Inspect TM Scopes

@matter123
Copy link
Collaborator

Could you try to reproduce this from a clean clone of the repo to try to determine if this is a build issue or if other changes might be affecting it?

@cjdb
Copy link
Author

cjdb commented Oct 18, 2019

Could you try to reproduce this from a clean clone of the repo to try to determine if this is a build issue or if other changes might be affecting it?

This didn't fix it. Perhaps we should jump on an IM channel? I'm available on cpplang.slack or #include<C++> Discord under @cjdb.

Is the pattern being recognized as a lambda? (Is the scope at { meta.function.definition.body.lambda.cpp when using Developer: Inspect TM Scopes

See attached.
image

@jeff-hykin
Copy link
Owner

I've been manually running npm test, but I just tried Launch and Build All: no difference.

Is it opening a separate window?
Running npm test will generate the files, but it won't change any visuals. To see the changes VS Code has to compile the extension, create a new window, and then sudo-install the newly compiled extension inside that window. And most times: when a change is made to the code the entire new-window needs to be reloaded.

Then new visuals and Developer: Inspect TM Scopes will only work inside that new window. Everywhere else will be the old version.

This didn't fix it. Perhaps we should jump on an IM channel?

Not a bad idea. Although if other people have this problem, we do want it to be findable.

@jeff-hykin
Copy link
Owner

just an update on this problem: in v1.14.20 (pushed just now) should add at least basic highlighting for those keywords

@HighCommander4
Copy link

just an update on this problem: in v1.14.20 (pushed just now) should add at least basic highlighting for those keywords

Awesome, thanks!

(Note: it does not highlight the second requires keyword from this example (the one that comes from a requires-clause rather than a requires-expression), but perhaps that's a known issue.)

@jeff-hykin
Copy link
Owner

jeff-hykin commented Jan 22, 2020

yeah... thanks for pointing that out. That case in particular is more of a templating problem see
#246: Template definition needs multiple fixes

@soroshsabz
Copy link
Contributor

related to #573
related to #575

@RedHolms
Copy link

Still open, and still doesn't work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists ✨ Enhancement New feature or request Syntax: C++ issue exists for C++
Projects
None yet
Development

No branches or pull requests

6 participants