stream: remove unreachable code - #18239
Conversation
Sorry, something went wrong.
To avoid a function call `BufferList.prototype.concat()` is not called when there is only a buffer in the list. That buffer is instead accessed directly.
The `n` argument of `BufferList.prototype.concat()` is not the number of `Buffer` instances in the list, but their total length when concatenated.
|
It's better to modify function fromList(n, state) in lib/_stream_readable.js by deleting the code, because BufferList is a independent module. |
Sorry, something went wrong.
|
ok. I agree. |
Sorry, something went wrong.
|
/cc @nodejs/streams |
Sorry, something went wrong.
|
I'm not super comfortable with this change. |
Sorry, something went wrong.
Do you mean if BufferList#concat() is called when there is only one element in the list and this patch is applied? If so, nothing, it will still work as expected just without the fast path (which is already here). |
Sorry, something went wrong.
|
Can you restore the test then? |
Sorry, something went wrong.
|
|
||
| const copy = list.concat(3); | ||
|
|
||
| assert.strictEqual(list.concat(1), 'foo'); |
There was a problem hiding this comment.
@mcollina this is wrong whether or not the change in BufferList.js is applied.
- The test uses a string, probably for simplicity, but concat() only works with buffers.
- The n argument (1) was wrong and irrelevant. You could have used any argument and the test would have still passed.
Sorry, something went wrong.
|
FWIW the fixed test actually tests the impossible case where BufferList#concat() is called and there is only one buffer in the list. Unlike the fast path, it creates a copy. |
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
LGTM, perfect
Sorry, something went wrong.
Sorry, something went wrong.
To avoid a function call `BufferList.prototype.concat()` is not called when there is only a buffer in the list. That buffer is instead accessed directly. PR-URL: #18239 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
The `n` argument of `BufferList.prototype.concat()` is not the number of `Buffer` instances in the list, but their total length when concatenated. PR-URL: #18239 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
To avoid a function call `BufferList.prototype.concat()` is not called when there is only a buffer in the list. That buffer is instead accessed directly. PR-URL: #18239 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
The `n` argument of `BufferList.prototype.concat()` is not the number of `Buffer` instances in the list, but their total length when concatenated. PR-URL: #18239 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
First commit remove some unreachable code. BufferList.prototype.concat() is not called when there is only a buffer in the list.
Second commit fixes a test.
Checklist
Affected core subsystem(s)
stream, test