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 — New Buffer API backport to v4.x by ChALkeR · Pull Request #7475 · nodejs/node

Conversation

@ChALkeR

@ChALkeR ChALkeR commented Jun 29, 2016 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

Description of change

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

Note: some backported tests are disabled, but those are not related to the new API.

Important: Buffer.from(arrayBuffer[, byteOffset [, length]]) (as in 6.x) was not backported, I used Buffer.from(arrayBuffer) instead, that helped to significantly reduce the code changes.

Note that --zero-fill-buffers is not included here (see #5745 for that), also the encoding parameter for .fill() is not included.

Existing code changes are minor here, this is not invasive.

Only two changes in existing code:

  1. .fill('') now zero-fills,

  2. One call to Buffer(string, encoding) is replaced with Buffer.from.

    There is a typecheck in that code path, so the behaviour is identical. In fact it could be reverted back to Buffer(string, encoding) or even changed to fromString(string, encoding) — that would just affect the double checks.

All the other changes are additive, i.e. introduce new methods and don't change the existing ones.

I'm hoping that this and #5745 will make it into 4.5.0.

/cc @jasnell @nodejs/ctc @seishun

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
seishun reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@addaleax @ChALkeR
buffer: ignore negative allocation lengths
f17b3e2 
Treat negative length arguments to `Buffer()`/`allocUnsafe()`
as if they were zero so the allocation does not affect the
pool’s offset.

Fixes: nodejs#7047

Refs: nodejs#7051
PR-URL: nodejs#7221
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@ChALkeR ChALkeR 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. ctc-agenda labels Jun 29, 2016
@ChALkeR ChALkeR changed the title Buffer v4.x New Buffer API backport to v4.x Jun 29, 2016
@ChALkeR
ChALkeR force-pushed the buffer-v4.x branch from a5c69a5 to 3d87f2d Compare June 29, 2016 13:31
@ChALkeR ChALkeR mentioned this pull request Jun 29, 2016
Node.js Foundation Core Technical Committee (CTC) Meeting 2016-06-29 #7474
Closed
@jasnell

jasnell commented Jun 29, 2016

Copy link
Copy Markdown
Member

LGTM if CI is green. One thing tho... the PR should be opened against v4.x-staging rather than directly against v4.x
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell mentioned this pull request Jun 29, 2016
buffer: backport --zero-fill-buffers command line option #5745
Closed
4 tasks done
@Fishrock123

Fishrock123 commented Jun 29, 2016

Copy link
Copy Markdown
Contributor

Hmmmm, I was under the impression that we settled on only backporting the flag? eeesh, I'm not even sure anymore. Dx
All reactions

Sorry, something went wrong.

Uh oh!

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

jasnell commented Jun 29, 2016

Copy link
Copy Markdown
Member

originally that was the intent but there have been requests from the user community to backport the APIs as well in order to make the transition easier.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

ChALkeR commented Jun 29, 2016

Copy link
Copy Markdown
Member Author

@jasnell Thanks! Will target v4.x-staging.

@Fishrock123 I was under an impression that the final decision was postponed (i.e. «will not backport at the moment»), and one of the reasons for that was that the changes were too invasive.

Some time has passed so things settled down a bit, it looks to me that there is a demand for this to be backported to v4, this is required so we could start speaking of (partial?) deprecation of the old API (see #7152), and it looke like there is going to be a v4 minor soon. Also, the changes here are non-invasive =).
All reactions

Sorry, something went wrong.

Uh oh!

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

seishun commented Jun 29, 2016

Copy link
Copy Markdown
Contributor

LGTM. Thanks for putting your time in this!
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

addaleax commented Jun 30, 2016

Copy link
Copy Markdown
Member

LGTM, CI: https://ci.nodejs.org/job/node-test-commit/3915/
All reactions

Sorry, something went wrong.

Uh oh!

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

rvagg commented Jun 30, 2016

Copy link
Copy Markdown
Member

👍 from CTC today
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rvagg rvagg removed the ctc-agenda label Jun 30, 2016
trevnorris
trevnorris reviewed Jun 30, 2016
View reviewed changes
Comment thread lib/buffer.js Outdated

@trevnorris trevnorris Jun 30, 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.

This comment doesn't make sense. So the function only has slightly and unusual cases?

Sorry, something went wrong.

Uh oh!

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

@ChALkeR ChALkeR Jul 1, 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.

Thanks, will remove that line.

Sorry, something went wrong.

Uh oh!

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

trevnorris commented Jun 30, 2016

Copy link
Copy Markdown
Contributor

One comment but LGTM otherwise.
All reactions

Sorry, something went wrong.

Uh oh!

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

MylesBorins commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

@ChALkeR are you going to close this and reopen? Would be nice to get into the v4.5.0-rc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins self-assigned this Jul 5, 2016
@ChALkeR
buffer: backport new buffer constructor APIs to v4.x
0a5487b 
This backports the new `Buffer.alloc()`, `Buffer.allocUnsafe()`,
`Buffer.from()`, and `Buffer.allocUnsafeSlow()` APIs for v4.

Some backported tests are disabled, but those are not related to the new API.
@ChALkeR
ChALkeR force-pushed the buffer-v4.x branch from 3d87f2d to 0a5487b Compare July 6, 2016 12:21
@ChALkeR ChALkeR mentioned this pull request Jul 6, 2016
New Buffer API backport to v4.x (staging branch) #7562
Closed
4 tasks done
@ChALkeR

ChALkeR commented Jul 6, 2016

Copy link
Copy Markdown
Member Author

Moved to #7562.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ChALkeR ChALkeR closed this Jul 6, 2016
ChALkeR pushed a commit to ChALkeR/io.js that referenced this pull request Jul 8, 2016
@addaleax @ChALkeR
buffer: ignore negative allocation lengths
18b5ef9 
Treat negative length arguments to `Buffer()`/`allocUnsafe()`
as if they were zero so the allocation does not affect the
pool’s offset.

Fixes: nodejs#7047

Refs: nodejs#7051
Refs: nodejs#7221
Refs: nodejs#7475
PR-URL: nodejs#7562
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Jul 8, 2016
@ChALkeR
buffer: backport new buffer constructor APIs to v4.x
1b39ebc 
This backports the new `Buffer.alloc()`, `Buffer.allocUnsafe()`,
`Buffer.from()`, and `Buffer.allocUnsafeSlow()` APIs for v4.

Some backported tests are disabled, but those are not related to the
new API.

Note that `Buffer.from(arrayBuffer[, byteOffset [, length]])` is not
supported in v4.x, only `Buffer.from(arrayBuffer)` is.

Refs: nodejs#4682
Refs: nodejs#5833
Refs: nodejs#7475
PR-URL: nodejs#7562
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 8, 2016
@addaleax Myles Borins
buffer: ignore negative allocation lengths
9ef71ab 
Treat negative length arguments to `Buffer()`/`allocUnsafe()`
as if they were zero so the allocation does not affect the
pool’s offset.

Fixes: #7047

Refs: #7051
Refs: #7221
Refs: #7475
PR-URL: #7562
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 8, 2016
@ChALkeR Myles Borins
buffer: backport new buffer constructor APIs to v4.x
34776ec 
This backports the new `Buffer.alloc()`, `Buffer.allocUnsafe()`,
`Buffer.from()`, and `Buffer.allocUnsafeSlow()` APIs for v4.

Some backported tests are disabled, but those are not related to the
new API.

Note that `Buffer.from(arrayBuffer[, byteOffset [, length]])` is not
supported in v4.x, only `Buffer.from(arrayBuffer)` is.

Refs: #4682
Refs: #5833
Refs: #7475
PR-URL: #7562
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
@addaleax Myles Borins
buffer: ignore negative allocation lengths
ee2f08f 
Treat negative length arguments to `Buffer()`/`allocUnsafe()`
as if they were zero so the allocation does not affect the
pool’s offset.

Fixes: #7047

Refs: #7051
Refs: #7221
Refs: #7475
PR-URL: #7562
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
@ChALkeR Myles Borins
buffer: backport new buffer constructor APIs to v4.x
b4a2791 
This backports the new `Buffer.alloc()`, `Buffer.allocUnsafe()`,
`Buffer.from()`, and `Buffer.allocUnsafeSlow()` APIs for v4.

Some backported tests are disabled, but those are not related to the
new API.

Note that `Buffer.from(arrayBuffer[, byteOffset [, length]])` is not
supported in v4.x, only `Buffer.from(arrayBuffer)` is.

Refs: #4682
Refs: #5833
Refs: #7475
PR-URL: #7562
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@addaleax Myles Borins
buffer: ignore negative allocation lengths
022439c 
Treat negative length arguments to `Buffer()`/`allocUnsafe()`
as if they were zero so the allocation does not affect the
pool’s offset.

Fixes: #7047

Refs: #7051
Refs: #7221
Refs: #7475
PR-URL: #7562
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@ChALkeR Myles Borins
buffer: backport new buffer constructor APIs to v4.x
ef3687f 
This backports the new `Buffer.alloc()`, `Buffer.allocUnsafe()`,
`Buffer.from()`, and `Buffer.allocUnsafeSlow()` APIs for v4.

Some backported tests are disabled, but those are not related to the
new API.

Note that `Buffer.from(arrayBuffer[, byteOffset [, length]])` is not
supported in v4.x, only `Buffer.from(arrayBuffer)` is.

Refs: #4682
Refs: #5833
Refs: #7475
PR-URL: #7562
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@addaleax Myles Borins
buffer: ignore negative allocation lengths
725c8c4 
Treat negative length arguments to `Buffer()`/`allocUnsafe()`
as if they were zero so the allocation does not affect the
pool’s offset.

Fixes: #7047

Refs: #7051
Refs: #7221
Refs: #7475
PR-URL: #7562
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@ChALkeR Myles Borins
buffer: backport new buffer constructor APIs to v4.x
12d9769 
This backports the new `Buffer.alloc()`, `Buffer.allocUnsafe()`,
`Buffer.from()`, and `Buffer.allocUnsafeSlow()` APIs for v4.

Some backported tests are disabled, but those are not related to the
new API.

Note that `Buffer.from(arrayBuffer[, byteOffset [, length]])` is not
supported in v4.x, only `Buffer.from(arrayBuffer)` is.

Refs: #4682
Refs: #5833
Refs: #7475
PR-URL: #7562
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@addaleax Myles Borins
buffer: ignore negative allocation lengths
200429e 
Treat negative length arguments to `Buffer()`/`allocUnsafe()`
as if they were zero so the allocation does not affect the
pool’s offset.

Fixes: #7047

Refs: #7051
Refs: #7221
Refs: #7475
PR-URL: #7562
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@ChALkeR Myles Borins
buffer: backport new buffer constructor APIs to v4.x
7090481 
This backports the new `Buffer.alloc()`, `Buffer.allocUnsafe()`,
`Buffer.from()`, and `Buffer.allocUnsafeSlow()` APIs for v4.

Some backported tests are disabled, but those are not related to the
new API.

Note that `Buffer.from(arrayBuffer[, byteOffset [, length]])` is not
supported in v4.x, only `Buffer.from(arrayBuffer)` is.

Refs: #4682
Refs: #5833
Refs: #7475
PR-URL: #7562
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
@MylesBorins MylesBorins removed their assignment Dec 27, 2016
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.

8 participants

@ChALkeR @jasnell @Fishrock123 @seishun @addaleax @rvagg @trevnorris @MylesBorins
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.