Skip to content

buffer: make byteLength work with ArrayBuffer & DataView#5255

Closed
JacksonTian wants to merge 1 commit intonodejs:masterfrom
JacksonTian:byteLength
Closed

buffer: make byteLength work with ArrayBuffer & DataView#5255
JacksonTian wants to merge 1 commit intonodejs:masterfrom
JacksonTian:byteLength

Conversation

@JacksonTian
Copy link
Contributor

@JacksonTian JacksonTian commented Feb 16, 2016

Convert anything to string, but Buffer, TypedArray and ArrayBuffer

@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Feb 16, 2016
@bnoordhuis
Copy link
Member

bnoordhuis commented Feb 16, 2016

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

@ChALkeR
Copy link
Member

ChALkeR commented Feb 16, 2016

Is this semver-minor or semver-major?

@jasnell
Copy link
Member

jasnell commented Feb 16, 2016

I'd say semver-minor
/cc @trevnorris

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 16, 2016
@bnoordhuis
Copy link
Member

bnoordhuis commented Feb 16, 2016

I'd say semver-minor as well, it's additive. Running the CI one more time to see if the ARM failures are transient: https://ci.nodejs.org/job/node-test-pull-request/1677/

@ChALkeR
Copy link
Member

ChALkeR commented Feb 16, 2016

Well, it actually changes the result on certain input values, but I guess that it would be safe to assume that noone is relying on the old behavior.

@trevnorris
Copy link
Contributor

trevnorris commented Feb 16, 2016

docs need to be updated with newly accepted arguments. implementation LGTM.

Mind making a comment in the commit message body about why this functionality is useful?

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

@JacksonTian ... ping...

@JacksonTian
Copy link
Contributor Author

JacksonTian commented Mar 22, 2016

Hi, @trevnorris @jasnell , I updated it.

jasnell
jasnell reviewed Mar 22, 2016
View reviewed changes
doc/api/buffer.markdown Outdated
Copy link
Member

@jasnell jasnell Mar 22, 2016

Choose a reason for hiding this comment

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

nit: {String | Buffer | DataView | ArrayBuffer}

Copy link
Contributor

@trevnorris trevnorris Mar 23, 2016

Choose a reason for hiding this comment

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

This doesn't explicitly mention the encompasing of all typed arrays. there a way that can be included?

Copy link
Contributor

@trevnorris trevnorris Mar 23, 2016

Choose a reason for hiding this comment

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

I ask this b/c everything listed here is a discrete constructor. Which at quick glance would show that all typed arrays are not supported.

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

LGTM with some nits

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

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

@JacksonTian
buffer: make byteLength work with ArrayBuffer & DataView
0c74f60 
Convert anything to string, but Buffer, TypedArray and ArrayBuffer

```
var uint8 = new Uint8Array([0xf0, 0x9f, 0x90]);
Buffer.byteLength(uint8); // should be 3, but returns 11
Buffer.byteLength(uint8.buffer); // should be 3, but return 20
```
@JacksonTian
Copy link
Contributor Author

JacksonTian commented Mar 23, 2016

Hi @trevnorris , I added the TypedArray links.

jasnell pushed a commit that referenced this pull request Mar 27, 2016
@JacksonTian @jasnell
buffer: make byteLength work with ArrayBuffer & DataView
293fd04 
Convert anything to string, but Buffer, TypedArray and ArrayBuffer

```
var uint8 = new Uint8Array([0xf0, 0x9f, 0x90]);
Buffer.byteLength(uint8); // should be 3, but returns 11
Buffer.byteLength(uint8.buffer); // should be 3, but return 20
```

PR-URL: #5255
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 27, 2016

Landed in 293fd04

@jasnell jasnell closed this Mar 27, 2016
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
@JacksonTian @evanlucas
buffer: make byteLength work with ArrayBuffer & DataView
ee83c95 
Convert anything to string, but Buffer, TypedArray and ArrayBuffer

```
var uint8 = new Uint8Array([0xf0, 0x9f, 0x90]);
Buffer.byteLength(uint8); // should be 3, but returns 11
Buffer.byteLength(uint8.buffer); // should be 3, but return 20
```

PR-URL: #5255
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas evanlucas mentioned this pull request Mar 31, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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.

6 participants

@JacksonTian @bnoordhuis @ChALkeR @jasnell @trevnorris @mscdex