Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Fixes #260, adding C++17 nested namespace decls. #263

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Anonymous-Stranger
Copy link

@Anonymous-Stranger Anonymous-Stranger commented Feb 28, 2018

First PR to this project, please let me know if I make any mistakes.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.

    • Hopefully okay
  • All new code requires tests to ensure against regressions

    • Where should I put the test? I tested the change on the following string:
      namespace blue {
      namespace abc::def {   // looks like github doesn't highlight it either
      namespace { 
      } } }

Description of the Change

Modified the regex for namespace declarations to allow the nested namespaces introduced in C++17.
DID NOT add/modify any tests. Could someone tell me what file the tests are in so that I can update it?

Alternate Designs

I played around with a couple of regexes before realizing that the one above it, the using-namespace declaration, had a regex that would work. Regex is already complex to understand, so I used that one to maintain symmetry. Not that this does create two branches of the same regex, which might be somewhat dangerous.

Benefits

We will (hopefully) have highlighting support for C++17 nested namespaces.

Possible Drawbacks

If the regex is wrong, I just broke syntax highlighting on a lot of peoples' computers. Also I'm creating two copies of the same regex, which is a bit dangerous.

Applicable Issues

#260

@iPherian
Copy link

iPherian commented May 4, 2018

looks good to me, for whatever that's worth.

The changes are identical to those for another pattern ('using namespace bla::bla') in the same file.

@iPherian
Copy link

iPherian commented May 4, 2018

pic for other working pattern:

language-c-highlighting-using-multi-component-ns

other comment in issue #260

@iPherian
Copy link

iPherian commented May 6, 2018

could @maxbrunsfeld look at this? I think it would be a good change.

@iPherian
Copy link

I'm not sure if there are other maintainers but here's some mentions

@kevinsawicki @MaximSokolov @50Wliu @as-cii @probablycorey

@Anonymous-Stranger
Copy link
Author

@iPherian, thanks for putting so much energy into getting this merged. While I understand your desire, I'm not sure it's a high priority PR. It neither changes much nor is it security related.

These are busy people and they will see it after they've finished all the other PRs also on their backlog. I'd prefer if we waited patiently and politely, but if you have a reason to see this as urgent, don't let me get in your way.

If you have a system that needs this fix, I think it's possible to edit the package on your machine, which would get you the fix ASAP.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants