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

test: fix RegExp nits #13770

Closed
wants to merge 5 commits into from
Closed

test: fix RegExp nits #13770

wants to merge 5 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Jun 18, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

It will be easier to review this PR commit by commit, performance-wise and for better comprehending (different changes are confusingly interwoven in the overall diff).

  1. Remove redundant RegExp part.

    RegExp part with a quantifier '0 or more times' is redundant if it is used at the very edge of the RegExp and is not captured or does not affect match indices (UPD: and should not greedily take up previous similar parts).

  2. Remove needless RegExp flag.

    In fixed case, /g flag is needless in the boolean context.

  3. Remove needless RegExp capturing.

    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.

  4. Use test(), not match() or exec() in boolean context.

    match() and exec() return a complicated object, unneeded in a boolean context. This fix also makes the code more clear and predictable.

  5. Do not needlessly repeat RegExp creation.

    This is the largest commit, however, it is rather easy to skim. It takes RegExp creation out of cycles and other repetitions.

    As long as the RegExp does not use /g flag + match indices, we are safe here.

    In tests, this fix hardly gives a significant performance gain, but it increases clarity and maintainability, reassuring some RegExps to be identical.

    RegExps in functions are not taken out of their functions: while these functions are called many times and their RegExps are recreated with each call, the performance gain in test cases does not seem to be worth decreasing function self-dependency.

RegExp part with a quantifier '0 or more' is redundant
if it is used at the very edge of the RegExp
and is not captured or does not affect match indices.
In fixed case, `/g` flag is needless in the boolean context.
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.
match() and exec() return a complicated object,
unneeded in a boolean context.
This commit takes RegExp creation out of cycles and other repetitions.

As long as the RegExp does not use /g flag and match indices,
we are safe here.

In tests, this fix hardly gives a significant performance gain,
but it increases clarity and maintainability,
reassuring some RegExps to be identical.

RegExp in functions are not taken out of their functions:
while these functions are called many times
and their RegExps are recreated with each call,
the performance gain in test cases
does not seem to be worth decreasing function self-dependency.
@nodejs-github-bot nodejs-github-bot added debugger doc Issues and PRs related to the documentations. inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jun 18, 2017
@vsemozhetbyt vsemozhetbyt removed debugger doc Issues and PRs related to the documentations. inspector Issues and PRs related to the V8 inspector protocol tools Issues and PRs related to the tools directory. labels Jun 18, 2017
@vsemozhetbyt
Copy link
Contributor Author

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if the CI passes (it's looking good so far).

@@ -22,7 +22,7 @@ function checkListResponse(instance, err, response) {
const match = wsUrl.match(/^ws:\/\/(.*):9229\/(.*)/);
assert.strictEqual(ip, match[1]);
assert.strictEqual(res['id'], match[2]);
assert.strictEqual(ip, res['devtoolsFrontendUrl'].match(/.*ws=(.*):9229/)[1]);
assert.strictEqual(ip, res['devtoolsFrontendUrl'].match(/ws=(.*):9229/)[1]);
Copy link
Contributor

@not-an-aardvark not-an-aardvark Jun 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change actually can have an effect on the match returned:

const string = 'ws=0.0.0.0:9229 ws=1.1.1.1:9229';

string.match(/.*ws=(.*):9229/)[1] // => '1.1.1.1'

string.match(/ws=(.*):9229/)[1] // => '0.0.0.0:9229 ws=1.1.1.1'

I'm not sure whether this matters for this particular test case.

[EDITED by @Trott to correct small typo in code: 9299 -> 9229]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I've missed this part can take up similar parts.

However, this RegExp checks a URL like:
chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=192.168.137.2:9229/151fc61d-6045-4692-b651-58bb5ba2c83d

If there is any chance this URL can have two ws= parts, let me know and I will remove this commit.

cc @nodejs/v8-inspector

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave it as is. If there's no good reason to make the test more lax, don't make it more lax. :-D

If the intention is to introduce a lint rule that would flag something like this line, it can always be disabled on just this line with a comment.

vsemozhetbyt added a commit that referenced this pull request Jun 21, 2017
* Remove needless RegExp flag

  In fixed case, `/g` flag is needless in the boolean context.

* Remove needless RegExp capturing

  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 test, not match/exec in boolean context

  match() and exec() return a complicated object,
  unneeded in a boolean context.

* Do not needlessly repeat RegExp creation

  This commit takes RegExp creation out of cycles and other repetitions.

  As long as the RegExp does not use /g flag and match indices,
  we are safe here.

  In tests, this fix hardly gives a significant performance gain,
  but it increases clarity and maintainability,
  reassuring some RegExps to be identical.

  RegExp in functions are not taken out of their functions:
  while these functions are called many times
  and their RegExps are recreated with each call,
  the performance gain in test cases
  does not seem to be worth decreasing function self-dependency.

PR-URL: #13770
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jun 21, 2017

Landed in 76340e3 (the first commit was dropped according to the request).

@vsemozhetbyt vsemozhetbyt deleted the test-regexp-nits branch June 21, 2017 00:50
@addaleax addaleax mentioned this pull request Jun 21, 2017
addaleax pushed a commit that referenced this pull request Jun 21, 2017
* Remove needless RegExp flag

  In fixed case, `/g` flag is needless in the boolean context.

* Remove needless RegExp capturing

  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 test, not match/exec in boolean context

  match() and exec() return a complicated object,
  unneeded in a boolean context.

* Do not needlessly repeat RegExp creation

  This commit takes RegExp creation out of cycles and other repetitions.

  As long as the RegExp does not use /g flag and match indices,
  we are safe here.

  In tests, this fix hardly gives a significant performance gain,
  but it increases clarity and maintainability,
  reassuring some RegExps to be identical.

  RegExp in functions are not taken out of their functions:
  while these functions are called many times
  and their RegExps are recreated with each call,
  the performance gain in test cases
  does not seem to be worth decreasing function self-dependency.

PR-URL: #13770
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax addaleax mentioned this pull request Jun 21, 2017
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

@vsemozhetbyt
Copy link
Contributor Author

@MylesBorins Backport: #14370

MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
* Remove needless RegExp flag

  In fixed case, `/g` flag is needless in the boolean context.

* Remove needless RegExp capturing

  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 test, not match/exec in boolean context

  match() and exec() return a complicated object,
  unneeded in a boolean context.

* Do not needlessly repeat RegExp creation

  This commit takes RegExp creation out of cycles and other repetitions.

  As long as the RegExp does not use /g flag and match indices,
  we are safe here.

  In tests, this fix hardly gives a significant performance gain,
  but it increases clarity and maintainability,
  reassuring some RegExps to be identical.

  RegExp in functions are not taken out of their functions:
  while these functions are called many times
  and their RegExps are recreated with each call,
  the performance gain in test cases
  does not seem to be worth decreasing function self-dependency.

Backport-PR-URL: #14370
PR-URL: #13770
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
* Remove needless RegExp flag

  In fixed case, `/g` flag is needless in the boolean context.

* Remove needless RegExp capturing

  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 test, not match/exec in boolean context

  match() and exec() return a complicated object,
  unneeded in a boolean context.

* Do not needlessly repeat RegExp creation

  This commit takes RegExp creation out of cycles and other repetitions.

  As long as the RegExp does not use /g flag and match indices,
  we are safe here.

  In tests, this fix hardly gives a significant performance gain,
  but it increases clarity and maintainability,
  reassuring some RegExps to be identical.

  RegExp in functions are not taken out of their functions:
  while these functions are called many times
  and their RegExps are recreated with each call,
  the performance gain in test cases
  does not seem to be worth decreasing function self-dependency.

Backport-PR-URL: #14370
PR-URL: #13770
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 21, 2017
MylesBorins pushed a commit that referenced this pull request Jul 31, 2017
* Remove needless RegExp flag

  In fixed case, `/g` flag is needless in the boolean context.

* Remove needless RegExp capturing

  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 test, not match/exec in boolean context

  match() and exec() return a complicated object,
  unneeded in a boolean context.

* Do not needlessly repeat RegExp creation

  This commit takes RegExp creation out of cycles and other repetitions.

  As long as the RegExp does not use /g flag and match indices,
  we are safe here.

  In tests, this fix hardly gives a significant performance gain,
  but it increases clarity and maintainability,
  reassuring some RegExps to be identical.

  RegExp in functions are not taken out of their functions:
  while these functions are called many times
  and their RegExps are recreated with each call,
  the performance gain in test cases
  does not seem to be worth decreasing function self-dependency.

Backport-PR-URL: #14370
PR-URL: #13770
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants