libs: fix some RegExp nits #13536
libs: fix some RegExp nits #13536
Conversation
nodejs-github-bot
added
dns
|
|
||
| servers.forEach((serv) => { | ||
| var ipVersion = isIP(serv); | ||
| if (ipVersion !== 0) | ||
| return newSet.push([ipVersion, serv]); | ||
|
|
||
| const match = serv.match(/\[(.*)\](?::\d+)?/); |
There was a problem hiding this comment.
I wonder if this was meant to be anchored, i.e. ^…$, otherwise the addition doesn’t make much sense? Anyway, your change seems correct.
There was a problem hiding this comment.
Maybe this was for some more clarity, to show the wider pattern. But this may impact performance a bit.
| @@ -772,7 +773,7 @@ function complete(line, callback) { | |||
| name = files[f]; | |||
| ext = path.extname(name); | |||
| base = name.slice(0, -ext.length); | |||
| if (base.match(/-\d+\.\d+(\.\d+)?/) || name === '.npm') { | |||
| if (versionedFileNamesRe.test(base) || name === '.npm') { | |||
There was a problem hiding this comment.
same here, but I really don’t know what this code is supposed to do. :/
There was a problem hiding this comment.
Ditto about clarity/performance )
There was a problem hiding this comment.
I looked into this a bit further and this all about the npm cache itself. It is meant to prevent listing all the version folders found in the cache folder (e.g. 1.0.0 in root/.npm/foo/1.0.0/package). As far as I see it this does not check the cache folder anymore though and the windows cache folder (npm-cache instead of .npm) is not excluded either. So I think this part could be safely removed and it's a legacy remnant. I did not try it out though.
* Take RegExp creation out of cycles. * Use test(), not match() in boolean context. * Remove redundant RegExp parts. PR-URL: #13536 Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
Landed in 390fa03 |
* Take RegExp creation out of cycles. * Use test(), not match() in boolean context. * Remove redundant RegExp parts. PR-URL: #13536 Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
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 |
|
@MylesBorins Backport: #14348 |
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
cluster, dns, repl, tls, util
Take
RegExpcreation out of cycles.I am not sure if v8 fully optimizes this now (= makes some
RegExps an identical one in cycles), so I've addressed this to be on the safe side. As long as theRegExpdoes not usegflag and match indices, we are safe here.Use
test(), notmatch()in boolean context.match()returns a complicated object, unneeded in a boolean context.Remove redundant
RegExpparts.If I get this correctly, a RegExp part with a quantifier '0 or more' is redundant if:
test());match()/exec()results (full or partial) or does not affect match indices.However, these parts may improve readability and comprehensibility. If this is the case for the changed fragments, feel free to reject the change.
These are rather trivial changes to cause any regressions, and I've tried not to cause any accidental deopts (being consistent with
var/constenvironment style in creating variables). If anybody believes some benchmarks are needed, please, tell me which of them I should run.It is easier to review this PR commit by commit, as the changes are interwoven and this may be confusing.