child_process: refactor internal/child_process.js#11366
child_process: refactor internal/child_process.js#11366notarseniy wants to merge 1 commit intonodejs:masterfrom
Conversation
lib/internal/child_process.js
Outdated
There was a problem hiding this comment.
Use var instead of let here for better backportability (let used in this way is still slower at least in v6.x and v4.x).
There was a problem hiding this comment.
This suggestion goes for all of the other occurrences below too.
There was a problem hiding this comment.
OK! Is there a guide on code style?
There was a problem hiding this comment.
@notarseniy make lint should have reported that issue. make lint is part of make test so if you ran tests, that should have come up as a lint error after the tests all passed. If the linter didn't report it, then there is probably a bug in our custom lint rule.
lib/internal/child_process.js
Outdated
There was a problem hiding this comment.
I'm not sure this change is really worth it, the previous conditional is shorter and is equivalent in behavior.
There was a problem hiding this comment.
Motivation of this change was this line with 'that-style' condition.
There was a problem hiding this comment.
Well IMHO that other line could/should be changed to a non-strict equality check. The performance difference probably isn't measurable anymore.
lib/internal/child_process.js
Outdated
There was a problem hiding this comment.
Instead of looking up the value each time, add a temporary variable before the conditional. For example:
const stdio = subprocess.stdio;
for (var i = 0; i < stdio.length; ++i) {
const stream = stdio[i];
// .... keep rest of the code the same
}There was a problem hiding this comment.
This suggestion goes for all of the other occurrences below too.
There was a problem hiding this comment.
Yeah, sounds good. Will fix this
There was a problem hiding this comment.
Good comment. Will fix.
lib/internal/child_process.js
Outdated
There was a problem hiding this comment.
Why this change? This would change behavior...
There was a problem hiding this comment.
In previous condition we're handling stdio === null, so this condition never will be executed with stdio === null
03eba26 to
0b3efdc
Compare
|
Commited with fixes of comments. |
|
More linting issues it seems. Run I'm guessing it's due to the duplicate |
lib/internal/child_process.js
Outdated
There was a problem hiding this comment.
This could probably be moved to the top of the function, where the first conditional can take advantage of it.
0b3efdc to
9bfcaf4
Compare
* Prefer === to == where possible * Remove condition that will always be false * Prefer for-loop statements to forEach where possible for perfomance reasons
9bfcaf4 to
efaa7d7
Compare
|
CI is green. LGTM. |
|
Bump! 😅 What the status of PR? Any other comments or additions? |
* Prefer === to == where possible * Remove condition that will always be false * Prefer for-loop statements to forEach where possible for perfomance reasons PR-URL: #11366 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
|
@notarseniy I think it has just been forgotten. I went ahead and landed it in 8fc362e. Thanks for the contribution! |
|
Thats okay :) Thank you too! |
* Prefer === to == where possible * Remove condition that will always be false * Prefer for-loop statements to forEach where possible for perfomance reasons PR-URL: #11366 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
* Prefer === to == where possible * Remove condition that will always be false * Prefer for-loop statements to forEach where possible for perfomance reasons PR-URL: #11366 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
* Prefer === to == where possible * Remove condition that will always be false * Prefer for-loop statements to forEach where possible for perfomance reasons PR-URL: #11366 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
* Prefer === to == where possible * Remove condition that will always be false * Prefer for-loop statements to forEach where possible for perfomance reasons PR-URL: #11366 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
* Prefer === to == where possible * Remove condition that will always be false * Prefer for-loop statements to forEach where possible for perfomance reasons PR-URL: #11366 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
child_process