★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21718Nouvelle recherche | Portail wanayoo
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.

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

doc: add documentation for buffer.byteOffset #21718

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
9 participants
@AndreasMadsen
Copy link
Member

AndreasMadsen commented Jul 9, 2018

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

Fixes: #19301

Checklist
doc: add documentation for buffer.byteOffset
Also document a common issue when casting a Buffer object to a
TypedArray object.

Fixes: #19301
@vsemozhetbyt
Copy link
Member

vsemozhetbyt left a comment

With some nits)

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

### buf.byteOffset

* {integer} The byteOffset on the underlying `ArrayBuffer` object based on

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jul 9, 2018

Member

byteOffset -> `byteOffset` or byte offset ?

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,

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jul 9, 2018

Member

an `TypedArray` -> a `TypedArray`.

in this case one needs to specify the `byteOffset` correctly:

```js
// create a buffer smaller than `Buffer.poolSize`

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jul 9, 2018

Member
  1. create -> Create?
  2. A period in the end?
// 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

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jul 9, 2018

Member
  1. node.js -> Node.js
  2. a Int8 -> an Int8
@TimothyGu
Copy link
Member

TimothyGu left a comment

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.

@lpinca

lpinca approved these changes Jul 9, 2018

@AndreasMadsen

This comment has been minimized.

Copy link
Member Author

AndreasMadsen commented Jul 9, 2018

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

This comment has been minimized.

Copy link
Member

TimothyGu commented Jul 9, 2018

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

This comment has been minimized.

Copy link
Member Author

AndreasMadsen commented Jul 10, 2018 •

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

This comment has been minimized.

Copy link
Member

BridgeAR commented Jul 13, 2018

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

@trivikr

This comment has been minimized.

Copy link
Contributor

trivikr commented Jul 16, 2018

@AndreasMadsen

This comment has been minimized.

Copy link
Member Author

AndreasMadsen commented Jul 16, 2018

Landed in b70367e

AndreasMadsen added a commit that referenced this pull request Jul 16, 2018

doc: add documentation for buffer.byteOffset
Also document a common issue when casting a Buffer object to a
TypedArray object.

Fixes: #19301

PR-URL: #21718
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

targos added a commit that referenced this pull request Jul 16, 2018

doc: add documentation for buffer.byteOffset
Also document a common issue when casting a Buffer object to a
TypedArray object.

Fixes: #19301

PR-URL: #21718
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

@targos targos referenced this pull request Jul 17, 2018

Merged

v10.7.0 proposal #21851

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.