Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

code cleanup #19896

Closed
wants to merge 2 commits into from
Closed

code cleanup #19896

wants to merge 2 commits into from

Conversation

vishal7201
Copy link
Contributor

@vishal7201 vishal7201 commented Apr 9, 2018 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Apr 9, 2018
richardlau
richardlau reviewed Apr 9, 2018
View reviewed changes
lib/_stream_readable.js Outdated
(state.needReadable ||
state.length < state.highWaterMark ||
state.length === 0);
(state.needReadable || state.length < state.highWaterMark);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can state.highWaterMark be 0? If so, < should probably be <=.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be <=.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add a test for this case.

@richardlau
Copy link
Member

cc @nodejs/streams

@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

@vishal7201 seems like something went wrong when updating your PR. Would you mind to rebase? :-)

BridgeAR
BridgeAR reviewed Apr 10, 2018
View reviewed changes
lib/_stream_readable.js Outdated
@@ -311,8 +311,7 @@ function chunkInvalid(state, chunk) {
// 'readable' event will be triggered.
function needMoreData(state) {
return !state.ended &&
(state.length < state.highWaterMark ||
state.length === 0);
(state.length < state.highWaterMark);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: this function could be inlined. It only has a single call site.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also needs to be <= as above

Copy link
Contributor Author

@vishal7201 vishal7201 Apr 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with <= , a test case is failing AssertionError [ERR_ASSERTION]: true strictEqual false at Object.<anonymous> (node/test/parallel/test-stream2-objects.js:212:12)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, this is ok.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove the parenthesis? They are not needed anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can prob also be a one liner as this looks like it's <80 chars

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. I will do that

mcollina
mcollina approved these changes Apr 11, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

MoonBall
MoonBall reviewed Apr 12, 2018
View reviewed changes
lib/_stream_readable.js
@@ -434,7 +432,7 @@ Readable.prototype.read = function(n) {
debug('need readable', doRead);

// 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch @MoonBall

@trivikr
Copy link
Member

trivikr commented Apr 15, 2018

This appears to be a fix for #19893

@vishal7201:

kodemill added a commit to kodemill/node that referenced this pull request May 28, 2018
@kodemill
stream: inline needMoreData function
ee2cbec 
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#L440

Fixes: nodejs#19893
Refs: nodejs#19896
@kodemill kodemill mentioned this pull request May 29, 2018
Closed
4 tasks
mcollina pushed a commit that referenced this pull request Jun 2, 2018
@kodemill @mcollina
stream: inline needMoreData function
1c07ebf 
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#L440

Fixes: #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>
MylesBorins pushed a commit that referenced this pull request Jun 6, 2018
@kodemill @MylesBorins
stream: inline needMoreData function
44fe78b 
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#L440

Fixes: #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
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MoonBall MoonBall MoonBall left review comments

@BridgeAR BridgeAR BridgeAR left review comments

@richardlau richardlau richardlau left review comments

@mafintosh mafintosh mafintosh left review comments

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@vishal7201 @richardlau @BridgeAR @trivikr @apapirovski @mcollina @mafintosh @MoonBall @nodejs-github-bot