doc: add documentation for buffer.byteOffset #21718

AndreasMadsen · 2018-07-09T14:23:07Z

Also document a common issue when casting a Buffer object to a
TypedArray object.

Fixes: #19301

Checklist

documentation is changed or added
commit message follows commit guidelines

Also document a common issue when casting a Buffer object to a TypedArray object. Fixes: #19301

nodejs-github-bot · 2018-07-09T14:23:08Z

@AndreasMadsen build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/228/pipeline

vsemozhetbyt

With some nits)

vsemozhetbyt · 2018-07-09T17:55:36Z

doc/api/buffer.md

@@ -992,6 +992,31 @@ console.log(buffer.buffer === arrayBuffer);
 // Prints: true
 ```

+### buf.byteOffset
+
+* {integer} The byteOffset on the underlying `ArrayBuffer` object based on


byteOffset -> `byteOffset` or byte offset ?

vsemozhetbyt · 2018-07-09T18:00:54Z

doc/api/buffer.md

+using `buf.buffer`, as the first bytes in this `ArrayBuffer` may be unrelated
+to the `buf` object itself.
+
+A common issue is when casting a `Buffer` object to an `TypedArray` object,


an `TypedArray` -> a `TypedArray`.

vsemozhetbyt · 2018-07-09T18:01:46Z

doc/api/buffer.md

+in this case one needs to specify the `byteOffset` correctly:
+
+```js
+// create a buffer smaller than `Buffer.poolSize`


create -> Create?

A period in the end?

vsemozhetbyt · 2018-07-09T18:02:59Z

doc/api/buffer.md

+// create a buffer smaller than `Buffer.poolSize`
+const nodeBuffer = new Buffer.from([0, 1, 2, 3, 4, 5, 6, 7, 8, 9]);
+
+// When casting the node.js Buffer to a Int8 TypedArray remember to use the


node.js -> Node.js

a Int8 -> an Int8

TimothyGu

Thanks for documenting how byteOffset is used, it's definitely something many people miss.

The complication here is that we don't document any of the other properties inherited from Uint8Array or %TypedArray% (except of, for which we supply our own implementation). Case in point is the byteLength property, which provides a uniform way to check the actual byte size of the array for any ArrayBuffer views and is encouraged for use in certain circumstances, even though it is equivalent to buf.length.

I'd prefer a caveat be put on the overload of Buffer.from that takes an ArrayBuffer, reminding people to supply an appropriate byteOffset and byteLength. Additionally I'd prefer the use of byteLength over buf.length as it is useful for making sure the code works for not just Buffer but any other typed array types.

AndreasMadsen · 2018-07-09T20:19:13Z

The complication here is that we don't document any of the other properties inherited from Uint8Array or %TypedArray% (except of, for which we supply our own implementation). Case in point is the byteLength property, which provides a uniform way to check the actual byte size of the array for any ArrayBuffer views and is encouraged for use in certain circumstances, even though it is equivalent to buf.length.

I don't think that is a complication. I also don't know the in what "certain circumstances" it is "encouraged" so I'm not really suited to document this. Feel free to open a PR yourself.

I'd prefer a caveat be put on the overload of Buffer.from that takes an ArrayBuffer, reminding people to supply an appropriate byteOffset and byteLength.

I would consider that an anti-pattern, since Buffer.from(buf.buffer, buf.byteOffset, buf.length) is the same as Buffer.from(buf). If you are referring to creating a buffer from a standalone ArrayBuffer, that has to do with Buffer.from(ArrayBuffer, byteOffset, length) not buf.byteOffset.

Additionally I'd prefer the use of byteLength over buf.length as it is useful for making sure the code works for not just Buffer but any other typed array types.

I honestly think that adds more confusion than value. The reader would wonder why .length is used everywhere else but not in this case.

TimothyGu · 2018-07-09T20:38:04Z

I also don't know the in what "certain circumstances" it is "encouraged" so I'm not really suited to document this.

In core at least we try to make all functions that take Buffer take any TypedArray. By "certain circumstances" I was referring to cases where any TypedArray is permitted.

since Buffer.from(buf.buffer, buf.byteOffset, buf.length) is the same as Buffer.from(buf)

There seems to be a miscommunication here. My understanding was that the former is a reference to buf's ArrayBuffer, while the second copies?

> aBuf = Buffer.alloc(10)
<Buffer 00 00 00 00 00 00 00 00 00 00>
> bBuf = Buffer.from(aBuf)
<Buffer 00 00 00 00 00 00 00 00 00 00>
> cBuf = Buffer.from(aBuf.buffer, aBuf.byteOffset, aBuf.byteLength)
<Buffer 00 00 00 00 00 00 00 00 00 00>
> aBuf[0] = 1
1
> aBuf
<Buffer 01 00 00 00 00 00 00 00 00 00>
> bBuf
<Buffer 00 00 00 00 00 00 00 00 00 00>
> cBuf
<Buffer 01 00 00 00 00 00 00 00 00 00>

My concern (which seems to be the same that drove you to open the PR) is that I've seen many people who use Buffer.from(buf.buffer) for getting another reference to the underlying data storage of buf, when they really mean Buffer.from(buf.buffer, buf.byteOffset, buf.byteLength).

The reader would wonder why .length is used everywhere else but not in this case.

Fair point.

AndreasMadsen · 2018-07-10T09:42:51Z

There seems to be a miscommunication here. My understanding was that the former is a reference to buf's ArrayBuffer, while the second copies?

That is fair. I will document that, but under Buffer.from(arraybuffer, byteOffset, length) or Buffer.from(buf).

edit: on second thought, why wouldn't you use buf.slice()?

BridgeAR · 2018-07-13T00:24:57Z

I have no strong opinion about adding it or not. But the change itself is LGTM with the comments addressed.

trivikr · 2018-07-16T14:13:39Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/911/

AndreasMadsen · 2018-07-16T14:23:10Z

CI: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/284/pipeline

AndreasMadsen · 2018-07-16T14:35:14Z

Landed in b70367e

Also document a common issue when casting a Buffer object to a TypedArray object. Fixes: #19301 PR-URL: #21718 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

doc: add documentation for buffer.byteOffset

316522a

Also document a common issue when casting a Buffer object to a TypedArray object. Fixes: #19301

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Jul 9, 2018

vsemozhetbyt approved these changes Jul 9, 2018

View reviewed changes

TimothyGu reviewed Jul 9, 2018

View reviewed changes

lpinca approved these changes Jul 9, 2018

View reviewed changes

jasnell approved these changes Jul 10, 2018

View reviewed changes

[squash] fix gramma

07a6baf

cjihrig approved these changes Jul 16, 2018

View reviewed changes

trivikr approved these changes Jul 16, 2018

View reviewed changes

AndreasMadsen closed this Jul 16, 2018

targos mentioned this pull request Jul 17, 2018

v10.7.0 proposal #21851

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: add documentation for buffer.byteOffset #21718

doc: add documentation for buffer.byteOffset #21718

AndreasMadsen commented Jul 9, 2018

nodejs-github-bot commented Jul 9, 2018

vsemozhetbyt left a comment

vsemozhetbyt Jul 9, 2018

vsemozhetbyt Jul 9, 2018

vsemozhetbyt Jul 9, 2018

vsemozhetbyt Jul 9, 2018

TimothyGu left a comment

AndreasMadsen commented Jul 9, 2018

TimothyGu commented Jul 9, 2018

AndreasMadsen commented Jul 10, 2018 •

edited

Loading

BridgeAR commented Jul 13, 2018

trivikr commented Jul 16, 2018

AndreasMadsen commented Jul 16, 2018

AndreasMadsen commented Jul 16, 2018

doc: add documentation for buffer.byteOffset #21718

doc: add documentation for buffer.byteOffset #21718

Conversation

AndreasMadsen commented Jul 9, 2018

Checklist

nodejs-github-bot commented Jul 9, 2018

vsemozhetbyt left a comment

Choose a reason for hiding this comment

vsemozhetbyt Jul 9, 2018

Choose a reason for hiding this comment

vsemozhetbyt Jul 9, 2018

Choose a reason for hiding this comment

vsemozhetbyt Jul 9, 2018

Choose a reason for hiding this comment

vsemozhetbyt Jul 9, 2018

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

AndreasMadsen commented Jul 9, 2018

TimothyGu commented Jul 9, 2018

AndreasMadsen commented Jul 10, 2018 • edited Loading

BridgeAR commented Jul 13, 2018

trivikr commented Jul 16, 2018

AndreasMadsen commented Jul 16, 2018

AndreasMadsen commented Jul 16, 2018

AndreasMadsen commented Jul 10, 2018 •

edited

Loading