-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Documentation update about Buffer initialization #9104
Conversation
@@ -64,7 +64,7 @@ It can be constructed in a variety of ways. | |||
|
|||
Allocates a new buffer of `size` octets. Note, `size` must be no more than | |||
[kMaxLength](smalloc.html#smalloc_smalloc_kmaxlength). Otherwise, a `RangeError` | |||
will be thrown here. | |||
will be thrown here. Unlike `ArrayBuffers`, the underlying memory for buffers is not initialized. So the contents of a newly created `Buffer` is unknown. Use `buf.fill(0)`to initialize a buffer to zeroes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just say:
The underlying memory of a Buffer
is not initialized. If this behavior is desired, use Buffer.prototype.fill()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the 'Unlike ArrayBuffers... ' part as suggested by another core dev. I think it helps. If its the length, I can paraphrase it further. But the Buffer.protoype.fill()
usage is inconsistent with rest of the documentation. Its gonna take a while to change all references to match it. What do you advice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a real need to mention ArrayBuffer
s at all, but I'm not strongly opposed to it. I guess it makes more sense to stick with the existing style of the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this?
Unlike, the underlying memory of a Buffer is not initialized. Use
buf.fill(0)
to initialize a buffer to zeroes.
There's a comparison with ArrayBuffer in the explanation for #slice
too. I'd say let's keep it. Ready to commit and push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tjfontaine thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please wrap at 80 chars.
One style nit, but LGTM otherwise. |
LGTM |
Landed in 70efdf3 |
fixes #7230