child_process: refactor internal/child_process.js#11366
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the child_process 
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.
@notarseniymake 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:
conststdio=subprocess.stdio;for(vari=0;i<stdio.length;++i){conststream=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
notarseniyforce-pushed the internal-child-process-fix branch from 03eba26 to 0b3efdcCompare
notarseniy commented Feb 14, 2017
Commited with fixes of comments. |
Trott commented Feb 14, 2017
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.
notarseniyforce-pushed the internal-child-process-fix branch from 0b3efdc to 9bfcaf4Compare
* Prefer === to == where possible * Remove condition that will always be false * Prefer for-loop statements to forEach where possible for perfomance reasons
notarseniyforce-pushed the internal-child-process-fix branch from 9bfcaf4 to efaa7d7Comparenotarseniy commented Feb 14, 2017
mscdex commented Feb 14, 2017
mscdex commented Feb 14, 2017
CI is green. LGTM. |
notarseniy commented Feb 18, 2017
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 <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Brian White <[email protected]>
targos commented Feb 18, 2017
@notarseniy I think it has just been forgotten. I went ahead and landed it in 8fc362e. Thanks for the contribution! |
notarseniy commented Feb 18, 2017
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 <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Brian White <[email protected]>
* 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 <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Brian White <[email protected]>
* 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 <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Brian White <[email protected]>
* 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 <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Brian White <[email protected]>
* 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 <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Brian White <[email protected]>
7 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
child_process