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

readline, repl, url, util: remove needless RegExp capturing #13720

Closed
wants to merge 1 commit into from
Closed

readline, repl, url, util: remove needless RegExp capturing #13720

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Jun 16, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

readline, repl, url, util

Use non-capturing grouping or remove capturing completely when:

  • capturing is useless per se, e.g. in test() check;
  • captured groups are not used afterward at all;
  • some of the later captured groups are not used afterward.

Use non-capturing grouping or remove capturing completely when:

* capturing is useless per se, e.g. in test() check;
* captured groups are not used afterwards at all;
* some of the later captured groups are not used afterwards.
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation. util Issues and PRs related to the built-in util module. labels Jun 16, 2017
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jun 16, 2017

@mscdex
Copy link
Contributor

mscdex commented Jun 16, 2017

Can't this be combined with #13718 ?

@refack
Copy link
Contributor

refack commented Jun 16, 2017

I'm +1 for explicitness. Do you know if this has any performance impact?

@refack
Copy link
Contributor

refack commented Jun 16, 2017

Can't this be combined with #13718 ?

I'm +1 on thinking about this (both have an impact on the benchmark footprint 🤔)

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jun 16, 2017

@refack I did not measure, but this seem to be commonplace, e.g.:

Capturing groups have a performance penalty. If you don't need the matched substring to be recalled, prefer non-capturing parentheses (see below).
MDN

@mscdex Do you mean I should close this PR and add it as a commit to the #13718?

@mscdex
Copy link
Contributor

mscdex commented Jun 16, 2017

@vsemozhetbyt Either way should work I think.

@vsemozhetbyt
Copy link
Contributor Author

Added to #13718.

@vsemozhetbyt vsemozhetbyt deleted the libs-no-needless-capture branch June 16, 2017 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. url Issues and PRs related to the legacy built-in url module. util Issues and PRs related to the built-in util module. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants