Uh oh!
There was an error while loading. Please reload this page.
code cleanup - #19896
Conversation
There was a problem hiding this comment.
Can state.highWaterMark be 0? If so, < should probably be <=.
There was a problem hiding this comment.
We should probably add a test for this case.
richardlau
commented
Apr 9, 2018
cc @nodejs/streams |
BridgeAR
commented
Apr 9, 2018
@vishal7201 seems like something went wrong when updating your PR. Would you mind to rebase? :-) |
| (state.length < state.highWaterMark || | ||
| state.length === 0); | ||
| (state.length < state.highWaterMark); | ||
| } |
There was a problem hiding this comment.
Suggestion: this function could be inlined. It only has a single call site.
There was a problem hiding this comment.
with <= , a test case is failing AssertionError [ERR_ASSERTION]: true strictEqual false at Object.<anonymous> (node/test/parallel/test-stream2-objects.js:212:12)
There was a problem hiding this comment.
can you remove the parenthesis? They are not needed anymore.
There was a problem hiding this comment.
can prob also be a one liner as this looks like it's <80 chars
…highWaterMark (needMoreData function)
| // if we currently have less than the highWaterMark, then also read some | ||
| if (state.length === 0 || state.length - n < state.highWaterMark) { | ||
| if (state.length - n < state.highWaterMark) { |
There was a problem hiding this comment.
Please note that state.highWaterMark, state.length and n maybe equal 0. So if they all equal 0, state.length - n < state.highWaterMark doesn't equal to state.length === 0 || state.length - n < state.highWaterMark.
I suggest that this change would be with another PR because code cleaning shouldn't change code logic.
trivikr
commented
Apr 15, 2018
This appears to be a fix for #19893
|
Inline the needMoreData function since it has only one call place. Update the related comment. Add a test for the edge case where HWM=0 and state.length=0. Add a test for ReadableStream.read(n) method's edge case where n, HWM and state.length are all zero. This proves that there is no easy way to simplify the check at https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L440Fixes: nodejs#19893 Refs: nodejs#19896
Inline the needMoreData function since it has only one call place. Update the related comment. Add a test for the edge case where HWM=0 and state.length=0. Add a test for ReadableStream.read(n) method's edge case where n, HWM and state.length are all zero. This proves that there is no easy way to simplify the check at https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L440Fixes: #19893 Refs: #19896 PR-URL: #21009 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Lance Ball <lball@redhat.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Inline the needMoreData function since it has only one call place. Update the related comment. Add a test for the edge case where HWM=0 and state.length=0. Add a test for ReadableStream.read(n) method's edge case where n, HWM and state.length are all zero. This proves that there is no easy way to simplify the check at https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L440Fixes: #19893 Refs: #19896 PR-URL: #21009 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Lance Ball <lball@redhat.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
apapirovski
commented
Jun 10, 2018
I'm going to close this out given that this has had no follow up in two months and there are issues with it, as expressed abbove. @vishal7201 please feel free to reopen if you would like to follow up on this! |
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes