buffer: do not emit deprecation notice on Buffer.of #19682
Conversation
nodejs-github-bot
added
the
buffer
lib/buffer.js
Outdated
| // | ||
| // Refs: https://tc39.github.io/ecma262/#sec-%typedarray%.of | ||
| Buffer.of = (...items) => { | ||
| const newObj = allocate(items.length); |
There was a problem hiding this comment.
Hm … we may want new FastBuffer(items.length) here, so that the returned buffer is not pooled, like you’d expect from the standard TypedArray methods.
Also, I’m curious, do you know why TypedArray.of does not use this[Symbol.species]? That seems like a good fix for this problem in the long run.
There was a problem hiding this comment.
Hmm, good point. I'll opt for createUnsafeBuffer() then, as we don't need the buffer to be zero-filled.
Good question. Let me ask: tc39/ecma262#1157
There was a problem hiding this comment.
Is it different from Buffer.from(items)?
There was a problem hiding this comment.
Yes; Buffer.of is (...items).
There was a problem hiding this comment.
That's already in the header, but is the implementation here different in behaviour from just returning Buffer.from(items)? Do we need to reimplement it?
|
@TimothyGu, no, my question above was — is there any reason not to define this just as |
lib/buffer.js
Outdated
| const newObj = createUnsafeBuffer(items.length); | ||
| for (var k = 0; k < items.length; k++) | ||
| newObj[k] = items[k]; | ||
| return newObj; |
There was a problem hiding this comment.
Line-to-line code duplication of fromArrayLike.
These four lines could be replaced with return fromArrayLike(items).
There was a problem hiding this comment.
No. fromArrayLike uses allocate() while this function uses createUnsafeBuffer(). The former may be pooled, which is not desirable.
lib/buffer.js
Outdated
| // Buffer() constructor. | ||
| // | ||
| // Refs: https://tc39.github.io/ecma262/#sec-%typedarray%.of | ||
| Buffer.of = (...items) => { |
There was a problem hiding this comment.
This changes the name of the function. It should probably be named 'of'.
There was a problem hiding this comment.
Grrr, I always thought this kind of assignment names the function automatically. Fixed.
There was a problem hiding this comment.
They are named in cases like { of: () => {} } and const of = () => {}, but not in x.of = () => {}.
Buffer.of.name === 'of'
|
@TimothyGu Ok, thanks for the updated answer re: difference from > Buffer.from([10]).buffer
ArrayBuffer { byteLength: 8192 }
> Buffer.of(10).buffer
ArrayBuffer { byteLength: 1 }Could you explain why is pooling the desired behaviour for Upd: ah, ok, nevermind — I suppose that the reason is that it speeds up things, but one could not expect pooling from |
| newObj[k] = items[k]; | ||
| return newObj; | ||
| }; | ||
| Buffer.of = of; |
There was a problem hiding this comment.
All the other functions in this file are defined as regular functions, not as arrow functions.
Is there a reason for specifically using an arrow function here? The non-arrow variant would have been one line less code.
There was a problem hiding this comment.
The reason is documented in the comment above :)
There was a problem hiding this comment.
You can also use a normal function and throw an explicit error if new.target is truthy.
There was a problem hiding this comment.
@ljharb That doesn’t work with https://esdiscuss.org/topic/isconstructor#content-11 :)
There was a problem hiding this comment.
(We would want the function to not be a constructor.)
There was a problem hiding this comment.
Yes, but there's not really a difference to users between whether it's got [[Construct]] or whether it throws on new.
|
|
||
| const common = require('../common'); | ||
|
|
||
| process.on('warning', common.mustNotCall()); |
There was a problem hiding this comment.
Should the --no-warnings flag be dropped?
There was a problem hiding this comment.
No, the --no-warnings just stops the console output. The process.on('warning') event will still emit.
BridgeAR
added
the
author ready
|
Landed in 42d8976. |
PR-URL: #19682 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #19682 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #19682 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes