New Buffer API backport to v4.x #7475

ChALkeR · 2016-06-29T13:30:18Z

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:

.fill('') now zero-fills,
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

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 <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2016-06-29T14:36:06Z

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

Fishrock123 · 2016-06-29T14:42:43Z

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

jasnell · 2016-06-29T14:47:02Z

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.

ChALkeR · 2016-06-29T15:05:43Z

@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 =).

seishun · 2016-06-29T19:50:28Z

LGTM. Thanks for putting your time in this!

addaleax · 2016-06-30T06:00:12Z

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

rvagg · 2016-06-30T06:50:27Z

👍 from CTC today

trevnorris · 2016-06-30T19:34:42Z

lib/buffer.js

+  if (typeof value === 'number')
+    throw new TypeError('"value" argument must not be a number');
+
+  // Slightly less common case.


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

Thanks, will remove that line.

trevnorris · 2016-06-30T19:35:16Z

One comment but LGTM otherwise.

MylesBorins · 2016-07-05T18:04:22Z

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

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 · 2016-07-06T12:33:36Z

Moved to #7562.

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 <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

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 <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

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 <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>

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 <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>