child_process: charify the usage of 'else' #11148
Conversation
nodejs-github-bot
added
the
child_process
lib/child_process.js
Outdated
|
|
||
| if (killSignal === 0) | ||
| throw new RangeError('"killSignal" cannot be 0'); | ||
| } else if (killSignal != null) { |
There was a problem hiding this comment.
What happened to the != null check?
There was a problem hiding this comment.
Oh, sorry, the killSignal can be false, undefined.
|
@JacksonTian there is a small typo in the commit message (it can be fixed when landed). |
|
What is the point of this |
|
@petercunha The |
|
Also, I noticed the typo in the commit message as well. Please remember to fix it now or on landing. |
|
@mscdex I understand that, it just seems entirely unnecessary. |
|
No opinion on this one, but if we are going to do it, might as well get the |
|
Ok. Let me fix line 38 too. |
the keyword 'else' is unnecessary after 'throw' statements.
| } | ||
|
|
||
| options = util._extend({}, arguments[pos++]); |
There was a problem hiding this comment.
This change means we access an out-of-bounds pos on arguments, which recent PRs have claimed is a deoptimization. Any concern with that?
There was a problem hiding this comment.
See #10323 for more context on my comment above
There was a problem hiding this comment.
there's actually a potentially out of bounds check immediately above. I'd recommend going through in a separate PR and dealing with those.
|
Thanks. Landed in dc9717c. |
The keyword 'else' is unnecessary after 'throw' statements. PR-URL: #11148 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
The keyword 'else' is unnecessary after 'throw' statements. PR-URL: #11148 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
|
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 |
the keyword 'else' is unnecessary after 'throw' statements.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
child_process