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.

Sign up for GitHub

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

Jump to bottom

buffer: make byteLength work with ArrayBuffer & DataView #5255

Closed
wants to merge 1 commit into from
Closed

buffer: make byteLength work with ArrayBuffer & DataView #5255

wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

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

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

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

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

Hi, @trevnorris @jasnell , I updated it.

jasnell
jasnell reviewed Mar 22, 2016
View reviewed changes
doc/api/buffer.markdown
@@ -468,7 +468,7 @@ additional performance that `Buffer.allocUnsafe(size)` provides.

### Class Method: Buffer.byteLength(string[, encoding])

* `string` {String}
* `string` {String|Buffer|DataView|ArrayBuffer}
Copy link
Member

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

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

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

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