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

buffer: backport new buffer constructor APIs #5763

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 17, 2016

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

@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
return fromString(arg, encoding);
if (typeof encodingOrOffset === 'string') {
throw new Error(
'If encoding is specified then the first argument must be a string'
Copy link
Member Author

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.

Copy link
Contributor

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

Copy link
Member Author

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.

@jasnell
Copy link
Member Author

jasnell commented Mar 21, 2016

@nodejs/collaborators ... ping

@trevnorris
Copy link
Contributor

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

@jasnell
Copy link
Member Author

jasnell commented Mar 22, 2016

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.

@jasnell
Copy link
Member Author

jasnell commented Mar 23, 2016

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

@jasnell
Copy link
Member Author

jasnell commented Mar 23, 2016

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

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
Copy link
Member Author

jasnell commented Mar 23, 2016

@jasnell
Copy link
Member Author

jasnell commented Mar 23, 2016

CI is green except for one unrelated failure.

jasnell added a commit that referenced this pull request Mar 24, 2016
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 <trev.norris@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Mar 24, 2016

Landed in v5.x in c1534e7

@jasnell jasnell closed this Mar 24, 2016
evanlucas added a commit that referenced this pull request Mar 31, 2016
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
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
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
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
## Buffers and ES6 iteration

Buffers can be iterated over using the ECMAScript 2015 (ES6) `for..of` syntax:

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

@c0b c0b Apr 24, 2016

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]); ??

@ChALkeR
Copy link
Member

ChALkeR commented May 5, 2016

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

@jasnell
Copy link
Member Author

jasnell commented May 5, 2016

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

@feross
Copy link
Contributor

feross commented May 30, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants