GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — buffer: backport new buffer constructor APIs by jasnell · Pull Request #5763 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented Mar 17, 2016

Copy link
Copy Markdown
Member

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

buffer

Description of change

This backports the new Buffer.alloc(), Buffer.allocUnsafe(), and Buffer.from() APIs for v5.

Also included in this backport is the change that allows fill('') to zero-fill (as opposed to doing nothing) and the additional byteOffset and length arguments for Buffer(arrayBuffer) and
Buffer.from(arrayBuffer).

This backport includes the new test cases.

This backport does not update all of the internal uses of the existing Buffer() constructor.

This backport also does not include the soft deprecation of the existing Buffer() constructor.

Refs: #5744
/cc: @Fishrock123 @trevnorris @nodejs/ctc @nodejs/lts

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ChALkeR reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@jasnell jasnell added buffer Issues and PRs related to the buffer subsystem. semver-minor PRs that contain new features and should be released in the next minor version. v5.x labels Mar 17, 2016
jasnell
jasnell reviewed Mar 17, 2016
View reviewed changes
Comment thread lib/buffer.js Outdated

@jasnell jasnell Mar 17, 2016

Copy link
Copy Markdown
Member Author

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.

Note: this particular bit is a semver-major change that was landed in master. I included it in the backport to ensure that the new api behavior in master would match what is backported. I can pull this new throw back out, however.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Fishrock123 Fishrock123 Mar 17, 2016

Copy link
Copy Markdown
Contributor

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.

yeah let's keep this purely additive

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell Mar 17, 2016

Copy link
Copy Markdown
Member Author

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.

Ok, pulled that throw back out and another. It's still not 100% "purely additive" in that it includes the change to fill('') to ensure it zero-fills as opposed to doing nothing.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell

jasnell commented Mar 21, 2016

Copy link
Copy Markdown
Member Author

@nodejs/collaborators ... ping
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@trevnorris

trevnorris commented Mar 21, 2016

Copy link
Copy Markdown
Contributor

Don't remember the discussion about back porting to v5.x, but assuming we did. LGTM.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the ctc-agenda label Mar 22, 2016
@jasnell

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member Author

Adding this to CTC Agenda for 2016-03-23 just to make sure there are no @nodejs/ctc objections to backporting the API changes to v5 and v4 and the --zero-fill-buffers flag back to v5, v4, v0.12 and v0.10. The plan discussed by the LTS WG is that the the --zero-fill-buffers and API backports could be landed in the lts staging branches but that the releases wouldn't happen until around the time v6 is cut.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rvagg rvagg mentioned this pull request Mar 23, 2016
Node.js Foundation Core Technical Committee (CTC) Meeting 2016-03-23 #5866
Closed
@jasnell jasnell removed the ctc-agenda label Mar 23, 2016
@jasnell

jasnell commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

Discussion on the CTC was to go ahead with in v5 but to hold off on backporting further back.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

@nodejs/ctc @nodejs/collaborators ... have one LGTM on this and no objections from CTC. However, it would be good to get some additional sign off. I'll give it 24 more hours for review then land tomorrow.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell added 2 commits March 23, 2016 14:49
@jasnell
buffer: backport new buffer constructor APIs
ad5e145 
This backports the new `Buffer.alloc()`, `Buffer.allocUnsafe()`,
and `Buffer.from()` APIs for v5.

Also included in this backport is the change that allows fill('')
to zero-fill (as opposed to doing nothing) and the additional
`byteOffset` and `length` arguments for `Buffer(arrayBuffer)` and
`Buffer.from(arrayBuffer)`.

This backport includes the new test cases.

This backport *does not* update all of the internal uses of the
existing `Buffer()` constructor.

This backport also *does not* include the soft deprecation of the
existing `Buffer()` constructor.
@jasnell
Pull out new throws
c04c7bd
@jasnell
jasnell force-pushed the v5.x-new-buffer-api branch from 6c5bc34 to c04c7bd Compare March 23, 2016 21:49
@jasnell

jasnell commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

New CI: https://ci.nodejs.org/job/node-test-pull-request/2048/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

CI is green except for one unrelated failure.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell added a commit that referenced this pull request Mar 24, 2016
@jasnell
buffer: backport new buffer constructor APIs
c1534e7 
This backports the new `Buffer.alloc()`, `Buffer.allocUnsafe()`,
and `Buffer.from()` APIs for v5.

Also included in this backport is the change that allows fill('')
to zero-fill (as opposed to doing nothing) and the additional
`byteOffset` and `length` arguments for `Buffer(arrayBuffer)` and
`Buffer.from(arrayBuffer)`.

This backport includes the new test cases.

This backport *does not* update all of the internal uses of the
existing `Buffer()` constructor.

This backport also *does not* include the soft deprecation of the
existing `Buffer()` constructor.

PR-URL: #5763
Reviewed-By: Trevor Norris <[email protected]>
@jasnell

jasnell commented Mar 24, 2016

Copy link
Copy Markdown
Member Author

Landed in v5.x in c1534e7
ChALkeR reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Mar 24, 2016
@evanlucas evanlucas mentioned this pull request Mar 31, 2016
v5.10.0 Release Proposal #5970
Closed
evanlucas added a commit that referenced this pull request Mar 31, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
9cd6e98 
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. (Forrest L Norvell)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Mar 31, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
1bb2c4b 
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Apr 1, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
79ea8c3 
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Apr 1, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
64c87e2 
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)

PR-URL: #5970
c0b
c0b reviewed Apr 24, 2016
View reviewed changes
Comment thread doc/api/buffer.markdown

```js
const buf = new Buffer([1, 2, 3]);
const buf = Buffer(.from[1, 2, 3]);

@c0b c0b Apr 24, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

this seems to be a typo; should be Buffer.from([1, 2, 3]); ??

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ChALkeR

ChALkeR commented May 5, 2016

Copy link
Copy Markdown
Member

@jasnell This missed the #5833 (well, because this PR was created earilier), this creates a minor inconsistency between 5.x and 6.x in the new API.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ChALkeR ChALkeR mentioned this pull request May 5, 2016
buffer: replace SlowBuffer with Buffer.allocUnsafeSlow(size) #5833
Closed
4 tasks done
@jasnell

jasnell commented May 5, 2016

Copy link
Copy Markdown
Member Author

Yeah, as far as I know #5833 hasn't been backported yet to v5. I'll open a PR for that soon
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@feross

feross commented May 30, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

@jasnell Any chance we can also backport to Node.js v4? Once v0.10 and and v0.12 are unsupported at the end of the year, it would be nice to start advising people to use only the new Buffer APIs (Buffer.from, etc.) in new code.

If we have support in v4, that would make shims like safe-buffer unnecessary and make it a no-brainer best practice.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@jasnell @trevnorris @ChALkeR @feross @Fishrock123 @c0b
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.