buffer: add encoding parameter to fill() #4935
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
trevnorris
added
buffer
jasnell
added
the
semver-minor
|
Marked as minor, might actually be a major due to the new throw. |
|
|
||
| if (str_length == 0) | ||
| return; | ||
|
|
||
| memcpy(ts_obj_data + start, *str, MIN(str_length, length)); | ||
| // Can't use StringBytes::Write() in all cases. For example if attempting | ||
| // to write a two bye character into a one byte Buffer. |
|
@jasnell Is that big endian? Looks like the bytes written are reversed. EDIT: yeah, I see now |
|
I believe so yeah.
|
| if (code < 256) | ||
| val = code; | ||
| } | ||
| if (typeof encoding === 'string' && !Buffer.isEncoding(encoding)) { |
There was a problem hiding this comment.
Do we want to throw if encoding is not a string?
There was a problem hiding this comment.
Guess that's easy enough to detect since end and start are both coerced if not strings. Since it's new API throwing won't be a breaking change anyway. I'll look at the rest of the Buffer API to see how it's handled there.
There was a problem hiding this comment.
Just checked. Seems write() and others throw for non-falsy values. Which means 0, NaN, null, etc. all get through. That does seem a little strange to me. Will make it throw for all values not undefined or typeof string.
trevnorris
force-pushed
the
buf-idxof-add
branch
from
44ecc2c to
0473f25
Compare
|
Added fix to swap bytes for
|
trevnorris
force-pushed
the
buf-idxof-add
branch
5 times, most recently
from
8bd6e9a to
ed58feb
Compare
|
@jasnell Haha! Fifth time's the charm. Everything is passing and comments have been addressed. Mind taking another look? |
trevnorris
force-pushed
the
buf-idxof-add
branch
2 times, most recently
from
a9ed9c3 to
7427c78
Compare
|
@bnoordhuis have any opinions here? |
|
hey sorry, finally back on this. LGTM |
trevnorris
force-pushed
the
buf-idxof-add
branch
from
7427c78 to
f7f69bb
Compare
Can now call fill() using following parameters if value is a String:
fill(string[, start[, end]][, encoding])
And with the following if value is a Buffer:
fill(buffer[, start[, end]])
The encoding is ignored if value is not a String. All other non-Buffer
values are coerced to a uint32.
A multibyte strings will simply be copied into the Buffer until the
number of bytes run out. Meaning partial strings can be left behind:
Buffer(3).fill('\u0222');
// returns: <Buffer c8 a2 c8>
In some encoding cases, such as 'hex', fill() will throw if the input
string is not valid.
PR-URL: nodejs#4935
Reviewed-By: James M Snell <jasnell@gmail.com>
trevnorris
force-pushed
the
buf-idxof-add
branch
from
f7f69bb to
b55e580
Compare
|
Thanks much for the review. Landed in b55e580. |
|
woo! next up, the Buffer constructor changes... that should be just about done also |
rvagg
pushed a commit
that referenced
this pull request
Feb 15, 2016
Can now call fill() using following parameters if value is a String:
fill(string[, start[, end]][, encoding])
And with the following if value is a Buffer:
fill(buffer[, start[, end]])
The encoding is ignored if value is not a String. All other non-Buffer
values are coerced to a uint32.
A multibyte strings will simply be copied into the Buffer until the
number of bytes run out. Meaning partial strings can be left behind:
Buffer(3).fill('\u0222');
// returns: <Buffer c8 a2 c8>
In some encoding cases, such as 'hex', fill() will throw if the input
string is not valid.
PR-URL: #4935
Reviewed-By: James M Snell <jasnell@gmail.com>
|
Uhoh it looks like b55e580 has broken the test suite for Literally every test is failing now 😢 /cc @trevnorris @rvagg @jasnell @indutny @nodejs/streams |
|
@thealphanerd got some test output we can see for what's actually breaking? we can either remove it off v5 and make a note in the breaking changes of v6 or we could try and fix it in situ |
|
Here is a gist of the output of npm test for https://gist.github.com/43e0cc7fd601ead3af42 @indutny is digging in right now and seems to think it might be a bug in the test suite. Whatever the case may be we have some sort of odd behavior change |
|
@thealphanerd If that's the case then it would have been an edge case for value coercion. I'm fine to fix that for backwards compatibility. |
|
here is a run of citgm testing node-spdy against v5.x |
rvagg
added a commit
that referenced
this pull request
Feb 21, 2016
* buffer:
- You can now supply an encoding argument when filling a
Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
an existing Buffer will also work with
Buffer#fill(buffer[, start[, end]]). See the API documentation for
details on how this works. (Trevor Norris) #4935
- Buffer#indexOf() no longer requires a byteOffset argument if you
also wish to specify an encoding:
Buffer#indexOf(val[, byteOffset][, encoding]).
(Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
to allow for optional execution of the given command inside a shell.
If set to true, cmd.exe will be used on Windows and /bin/sh
elsewhere. A path to a custom shell can also be passed to override
these defaults. On Windows, this option allows .bat. and .cmd files
to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
strict limitation of allowable header characters.
(James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
Strings as the first argument. See the API docs for details on how
this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
'upgrade' event where the server is just advertising its protocols.
This bug can prevent HTTP clients from communicating with HTTP/2
enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
indicate whether the server is listening for connections.
(José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
it from inside another MakeCallback() call no longer causes the
nextTick queue or Promises microtask queue to be processed out of
order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
new vm.Script() to interact with V8's code cache. When a new
vm.Script object is created with the 'produceCachedData' set to true
a Buffer with V8's code cache data will be produced and stored in
cachedData property of the returned object. This data in turn may be
supplied back to another vm.Script() object with a 'cachedData'
option if the supplied source is the same. Successfully executing a
script from cached data can speed up instantiation time. See the API
docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
- process.nextTick() (Ruben Bridgewater) #5092
- path module (Brian White) #5123
- querystring module (Brian White) #5012
- streams module when processing small chunks (Matteo Collina) #4354
rvagg
added a commit
that referenced
this pull request
Feb 21, 2016
* buffer:
- You can now supply an encoding argument when filling a
Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
an existing Buffer will also work with
Buffer#fill(buffer[, start[, end]]). See the API documentation for
details on how this works. (Trevor Norris) #4935
- Buffer#indexOf() no longer requires a byteOffset argument if you
also wish to specify an encoding:
Buffer#indexOf(val[, byteOffset][, encoding]).
(Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
to allow for optional execution of the given command inside a shell.
If set to true, cmd.exe will be used on Windows and /bin/sh
elsewhere. A path to a custom shell can also be passed to override
these defaults. On Windows, this option allows .bat. and .cmd files
to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
strict limitation of allowable header characters.
(James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
Strings as the first argument. See the API docs for details on how
this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
'upgrade' event where the server is just advertising its protocols.
This bug can prevent HTTP clients from communicating with HTTP/2
enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
indicate whether the server is listening for connections.
(José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
it from inside another MakeCallback() call no longer causes the
nextTick queue or Promises microtask queue to be processed out of
order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
new vm.Script() to interact with V8's code cache. When a new
vm.Script object is created with the 'produceCachedData' set to true
a Buffer with V8's code cache data will be produced and stored in
cachedData property of the returned object. This data in turn may be
supplied back to another vm.Script() object with a 'cachedData'
option if the supplied source is the same. Successfully executing a
script from cached data can speed up instantiation time. See the API
docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
- process.nextTick() (Ruben Bridgewater) #5092
- path module (Brian White) #5123
- querystring module (Brian White) #5012
- streams module when processing small chunks (Matteo Collina) #4354
rvagg
added a commit
that referenced
this pull request
Feb 23, 2016
* buffer:
- You can now supply an encoding argument when filling a
Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
an existing Buffer will also work with
Buffer#fill(buffer[, start[, end]]). See the API documentation for
details on how this works. (Trevor Norris) #4935
- Buffer#indexOf() no longer requires a byteOffset argument if you
also wish to specify an encoding:
Buffer#indexOf(val[, byteOffset][, encoding]).
(Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
to allow for optional execution of the given command inside a shell.
If set to true, cmd.exe will be used on Windows and /bin/sh
elsewhere. A path to a custom shell can also be passed to override
these defaults. On Windows, this option allows .bat. and .cmd files
to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
strict limitation of allowable header characters.
(James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
Strings as the first argument. See the API docs for details on how
this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
'upgrade' event where the server is just advertising its protocols.
This bug can prevent HTTP clients from communicating with HTTP/2
enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
indicate whether the server is listening for connections.
(José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
it from inside another MakeCallback() call no longer causes the
nextTick queue or Promises microtask queue to be processed out of
order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
new vm.Script() to interact with V8's code cache. When a new
vm.Script object is created with the 'produceCachedData' set to true
a Buffer with V8's code cache data will be produced and stored in
cachedData property of the returned object. This data in turn may be
supplied back to another vm.Script() object with a 'cachedData'
option if the supplied source is the same. Successfully executing a
script from cached data can speed up instantiation time. See the API
docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
- process.nextTick() (Ruben Bridgewater) #5092
- path module (Brian White) #5123
- querystring module (Brian White) #5012
- streams module when processing small chunks (Matteo Collina) #4354
PR-URL: #5295
stefanmb
pushed a commit
to stefanmb/node
that referenced
this pull request
Feb 23, 2016
Can now call fill() using following parameters if value is a String:
fill(string[, start[, end]][, encoding])
And with the following if value is a Buffer:
fill(buffer[, start[, end]])
The encoding is ignored if value is not a String. All other non-Buffer
values are coerced to a uint32.
A multibyte strings will simply be copied into the Buffer until the
number of bytes run out. Meaning partial strings can be left behind:
Buffer(3).fill('\u0222');
// returns: <Buffer c8 a2 c8>
In some encoding cases, such as 'hex', fill() will throw if the input
string is not valid.
PR-URL: nodejs#4935
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg
added a commit
that referenced
this pull request
Feb 24, 2016
* buffer:
- You can now supply an encoding argument when filling a
Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
an existing Buffer will also work with
Buffer#fill(buffer[, start[, end]]). See the API documentation for
details on how this works. (Trevor Norris) #4935
- Buffer#indexOf() no longer requires a byteOffset argument if you
also wish to specify an encoding:
Buffer#indexOf(val[, byteOffset][, encoding]).
(Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
to allow for optional execution of the given command inside a shell.
If set to true, cmd.exe will be used on Windows and /bin/sh
elsewhere. A path to a custom shell can also be passed to override
these defaults. On Windows, this option allows .bat. and .cmd files
to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
strict limitation of allowable header characters.
(James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
Strings as the first argument. See the API docs for details on how
this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
'upgrade' event where the server is just advertising its protocols.
This bug can prevent HTTP clients from communicating with HTTP/2
enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
indicate whether the server is listening for connections.
(José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
it from inside another MakeCallback() call no longer causes the
nextTick queue or Promises microtask queue to be processed out of
order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
new vm.Script() to interact with V8's code cache. When a new
vm.Script object is created with the 'produceCachedData' set to true
a Buffer with V8's code cache data will be produced and stored in
cachedData property of the returned object. This data in turn may be
supplied back to another vm.Script() object with a 'cachedData'
option if the supplied source is the same. Successfully executing a
script from cached data can speed up instantiation time. See the API
docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
- process.nextTick() (Ruben Bridgewater) #5092
- path module (Brian White) #5123
- querystring module (Brian White) #5012
- streams module when processing small chunks (Matteo Collina) #4354
PR-URL: #5295
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Can now call fill() using following parameters if value is a String:
And with the following if value is a Buffer:
If value is not a String then encoding is ignored. All other non-Buffer
values are coerced to a uint32.
A multibyte strings will simply be copied into the Buffer until the
number of bytes run out. Meaning partial strings can be left behind:
In some encoding cases, such as 'hex', fill() will throw if the input
string is not valid.
R=@bnoordhuis
R=@jasnell
CI: https://ci.nodejs.org/job/node-test-pull-request/1422/