Skip to content

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