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.

Sign up for GitHub

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

Jump to bottom

Add SharedArrayBuffer to Buffer documentation #15489

Closed
wants to merge 5 commits into from
Closed

Add SharedArrayBuffer to Buffer documentation #15489

wants to merge 5 commits into from

Conversation

ThomasdenH
Copy link
Contributor

Since 2a2ec9d, Buffer accepts a SharedArrayBuffer in places where ArrayBuffer was accepted previously. This PR updates the documentation accordingly.

Note that not every occurrence was replaced. In most cases it seems clear from the context that both are accepted, although some might disagree about this.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Sep 20, 2017
lpinca
lpinca reviewed Sep 20, 2017
View reviewed changes
doc/api/buffer.md Outdated
@@ -2702,6 +2703,7 @@ This value may depend on the JS engine that is being used.

[`ArrayBuffer#slice()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer/slice
[`ArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer
[`SharedArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please move this line to keep the alphabetical order?

@ThomasdenH
Copy link
Contributor Author

Done. I can rebase the commits if you prefer.

@lpinca
Copy link
Member

lpinca commented Sep 20, 2017

No, it's fine, commits are squashed before landing. Thank you.

TimothyGu
TimothyGu reviewed Sep 20, 2017
View reviewed changes
doc/api/buffer.md Outdated
@@ -361,16 +361,16 @@ changes:
> [`Buffer.from(arrayBuffer[, byteOffset [, length]])`][`Buffer.from(arrayBuffer)`]
> instead.

* `arrayBuffer` {ArrayBuffer} An [`ArrayBuffer`] or the `.buffer` property of a
[`TypedArray`].
* `arrayBuffer` {ArrayBuffer} An [`ArrayBuffer`], [`SharedArrayBuffer`] or the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change {ArrayBuffer} to {ArrayBuffer|SharedArrayBuffer} as well?

doc/api/buffer.md
When `string` is a `Buffer`/[`DataView`]/[`TypedArray`]/[`ArrayBuffer`], the
actual byte length is returned.
When `string` is a `Buffer`/[`DataView`]/[`TypedArray`]/[`ArrayBuffer`]/
[`SharedArrayBuffer`], the actual byte length is returned.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

doc/api/buffer.md Outdated
* `arrayBuffer` {ArrayBuffer} An [`ArrayBuffer`] or the `.buffer` property of a
[`TypedArray`].
* `arrayBuffer` {ArrayBuffer} An [`ArrayBuffer`], [`SharedArrayBuffer`], or the
`.buffer` property of a [`TypedArray`].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

BridgeAR
BridgeAR approved these changes Sep 24, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the comments addressed.

@ThomasdenH
Copy link
Contributor Author

I've addressed the comments.

BridgeAR
BridgeAR reviewed Sep 24, 2017
View reviewed changes
doc/api/buffer.md Outdated
@@ -807,8 +807,8 @@ A `TypeError` will be thrown if `array` is not an `Array`.
added: v5.10.0
-->

* `arrayBuffer` {ArrayBuffer} An [`ArrayBuffer`] or the `.buffer` property of a
[`TypedArray`].
* `arrayBuffer` {ArrayBuffer|SharedArrayBuffer} An [`ArrayBuffer`], [`SharedArrayBuffer`], or the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line length is above 80 chars.

BridgeAR
BridgeAR reviewed Sep 24, 2017
View reviewed changes
doc/api/buffer.md Outdated
@@ -679,7 +679,7 @@ changes:
or `ArrayBuffer`.
-->

* `string` {string|Buffer|TypedArray|DataView|ArrayBuffer} A value to
* `string` {string|Buffer|TypedArray|DataView|ArrayBuffer|SharedArrayBuffer} A value to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line length is above 80 chars.

BridgeAR
BridgeAR reviewed Sep 24, 2017
View reviewed changes
doc/api/buffer.md Outdated
@@ -361,16 +361,16 @@ changes:
> [`Buffer.from(arrayBuffer[, byteOffset [, length]])`][`Buffer.from(arrayBuffer)`]
> instead.

* `arrayBuffer` {ArrayBuffer} An [`ArrayBuffer`] or the `.buffer` property of a
[`TypedArray`].
* `arrayBuffer` {ArrayBuffer|SharedArrayBuffer} An [`ArrayBuffer`], [`SharedArrayBuffer`] or the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line length is above 80 chars.

@BridgeAR
Copy link
Member

BridgeAR commented Oct 1, 2017

Ping @ThomasdenH this needs a rebase and please address the comments about the line length.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 22, 2017
@BridgeAR
Copy link
Member

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

@jasnell
Copy link
Member

jasnell commented Nov 22, 2017

@ThomasdenH ... can I ask you to please squash the commits down into a single commit

@targos
Copy link
Member

targos commented Nov 23, 2017 •
edited
Loading

Linter CI after rebase and squash: https://ci.nodejs.org/job/node-test-linter/13860/

Edit: CI timed out. New try: https://ci.nodejs.org/job/node-test-linter/13861/

@targos
Copy link
Member

targos commented Nov 23, 2017

Landed in bd0e36d

@targos targos closed this Nov 23, 2017
targos pushed a commit that referenced this pull request Nov 23, 2017
@ThomasdenH @targos
doc: add SharedArrayBuffer to Buffer documentation
bd0e36d 
PR-URL: #15489
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@joyeecheung joyeecheung mentioned this pull request Nov 23, 2017
Merged
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@ThomasdenH @MylesBorins
doc: add SharedArrayBuffer to Buffer documentation
118a3e2 
PR-URL: #15489
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@ThomasdenH @MylesBorins
doc: add SharedArrayBuffer to Buffer documentation
df5436c 
PR-URL: #15489
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@ThomasdenH @gibfahn
doc: add SharedArrayBuffer to Buffer documentation
cdb7d24 
PR-URL: #15489
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@ThomasdenH @gibfahn
doc: add SharedArrayBuffer to Buffer documentation
a0bd1c0 
PR-URL: #15489
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@ThomasdenH @gibfahn
doc: add SharedArrayBuffer to Buffer documentation
36f5f13 
PR-URL: #15489
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
@MylesBorins MylesBorins mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca left review comments

@TimothyGu TimothyGu TimothyGu left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@jasnell jasnell jasnell approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.