buffer: Added Buffer#includes() to keep parity with TypedArray. - #3567
buffer: Added Buffer#includes() to keep parity with TypedArray.#3567umamialex wants to merge 1 commit into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
const instead of var please
Sorry, something went wrong.
|
Switched var to const where possible. |
Sorry, something went wrong.
|
Isn't this a little premature? TypedArray#includes() is still behind a flag in the V8 we ship, isn't it? |
Sorry, something went wrong.
|
@bnoordhuis v8's implementation of this and other methods like fill(), indexOf(), etc. are all restrictive and/or slow, and we've already replaced them with our own implementation. So figured it would be fine to implement this now using our faster implementation since the change was relatively minimal. |
Sorry, something went wrong.
|
@suitupalex Excellent job on all the tests. LGTM if CI is happy. |
Sorry, something went wrong.
|
Mind adding docs for it? |
Sorry, something went wrong.
Simply adds Buffer.prototype.includes by wrapping an indexOf and performing a strict equals check to -1. The includes method takes the search value, byteOffset, and encoding as arguments. The test is a modified version of the indexOf test. Addresses #3552.
|
@trevnorris thanks! Glad to be able to contribute. @evanlucas absolutely. Just reupped with documentation. |
Sorry, something went wrong.
|
CI looks good. Those failures look unrelated. It looks like the PPC machines are having some trouble though? /cc @nodejs/build |
Sorry, something went wrong.
|
LGTM |
Sorry, something went wrong.
|
Even though we're bypassing V8 on this I'd be in favour of waiting till TypedArray#includes() has landed stable upstream. There's a chance we could be overlooking something and it would be good to be able to test our implementation alongside theirs so we get maximum coverage of the requirements. The fact that they still have it in staging suggests (to me, without any actual knowledge of why) that it's non-trivial. |
Sorry, something went wrong.
|
^ the main reason for waiting would be to ensure that we don't ship something we have to break in a later version because we've overlooked something. |
Sorry, something went wrong.
|
@rvagg I'm curious what you could mean? Our implementations all break ECMA standards (b/c honestly they suck in this case), so I'm not sure what we should be worried about. |
Sorry, something went wrong.
|
includes was specifically designed not to suck so I'd at least like to see some benchmarks before creating an overriden version... |
Sorry, something went wrong.
|
@domenic but you can still only pass a number, right? Buffer#includes() should allow multi character strings or Buffers to be passed in like similar existing APIs in core. In this case it's not only about performance. |
Sorry, something went wrong.
|
Why? That isn't what "includes" means in ES. |
Sorry, something went wrong.
|
It would be similar to String#includes: new Buffer('abcd').includes('bc') <=> 'abcd'.includes('bc') |
Sorry, something went wrong.
|
@domenic Keep in mind this isn't TypedArray.includes -- this is for buffer, which acts more like a string in some cases. |
Sorry, something went wrong.
|
Is anything blocking this? |
Sorry, something went wrong.
|
Nothing from me. |
Sorry, something went wrong.
|
LGTM |
Sorry, something went wrong.
|
Perhaps the approach to take on this is to get it landed in master but hold off just a bit before landing it in v5.x? The change itself LGTM |
Sorry, something went wrong.
|
@Fishrock123 Follow up, can this be merged? |
Sorry, something went wrong.
|
@trevnorris is the agreement to ship in master but not land it in a stable until TypedArray adds it? |
Sorry, something went wrong.
|
@Fishrock123 Not sure. Since this disregards the Typed Array spec I was under the impression that it wouldn't matter. Let's land this on master and discuss back porting after. |
Sorry, something went wrong.
Add Buffer#includes() by wrapping an indexOf and performing a strict equals check to -1. The includes method takes the search value, byteOffset, and encoding as arguments. The test is a modified version of the indexOf test. Fixes: #3552 PR-URL: #3567 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add Buffer#includes() by wrapping an indexOf and performing a strict equals check to -1. The includes method takes the search value, byteOffset, and encoding as arguments. The test is a modified version of the indexOf test. Fixes: #3552 PR-URL: #3567 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Notable changes:
* buffer:
- Buffer.prototype.includes() has been added to keep parity
with TypedArrays. (Alexander Martin) #3567.
* domains:
- Fix handling of uncaught exceptions.
(Julien Gilli) #3654.
* https:
- Added support for disabling session caching.
(Fedor Indutny) #4252.
* repl:
- Allow third party modules to be imported using
require(). This corrects a regression from 5.2.0.
(Ben Noordhuis) #4215.
* deps:
- Upgrade libuv to 1.8.0.
(Saúl Ibarra Corretgé) #4276.
PR-URL: #4281
Notable changes:
* buffer:
- Buffer.prototype.includes() has been added to keep parity
with TypedArrays. (Alexander Martin) #3567.
* domains:
- Fix handling of uncaught exceptions.
(Julien Gilli) #3654.
* https:
- Added support for disabling session caching.
(Fedor Indutny) #4252.
* repl:
- Allow third party modules to be imported using
require(). This corrects a regression from 5.2.0.
(Ben Noordhuis) #4215.
* deps:
- Upgrade libuv to 1.8.0.
(Saúl Ibarra Corretgé) #4276.
PR-URL: #4281
Conflicts:
src/node_version.h
Add Buffer#includes() by wrapping an indexOf and performing a strict equals check to -1. The includes method takes the search value, byteOffset, and encoding as arguments. The test is a modified version of the indexOf test. Fixes: nodejs#3552 PR-URL: nodejs#3567 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Notable changes:
* buffer:
- Buffer.prototype.includes() has been added to keep parity
with TypedArrays. (Alexander Martin) nodejs#3567.
* domains:
- Fix handling of uncaught exceptions.
(Julien Gilli) nodejs#3654.
* https:
- Added support for disabling session caching.
(Fedor Indutny) nodejs#4252.
* repl:
- Allow third party modules to be imported using
require(). This corrects a regression from 5.2.0.
(Ben Noordhuis) nodejs#4215.
* deps:
- Upgrade libuv to 1.8.0.
(Saúl Ibarra Corretgé) nodejs#4276.
PR-URL: nodejs#4281
Conflicts:
src/node_version.h
Some of the tests for `buffer.includes()` functionality introduced in nodejs#3567 have been broken in a way that caused them to always pass regardless of the result of the tested method. This behavior was caused by two reasons: * These tests were written as though `buffer.includes()` was supposed to return the same value that `buffer.indexOf()` does, i.e., used indices or -1 as expected return values instead of true and false. * `assert()` was used as the assertion function to do that instead of `assert.strictEqual()`. Thus `assert()` was called with a non-zero number as the first argument effectively causing these tests to pass. This commit changes the tests to use `assert.ok()` and removes redundant indices. Refs: nodejs#3567
Some of the tests for `buffer.includes()` functionality introduced in #3567 have been broken in a way that caused them to always pass regardless of the result of the tested method. This behavior was caused by two reasons: * These tests were written as though `buffer.includes()` was supposed to return the same value that `buffer.indexOf()` does, i.e., used indices or -1 as expected return values instead of true and false. * `assert()` was used as the assertion function to do that instead of `assert.strictEqual()`. Thus `assert()` was called with a non-zero number as the first argument effectively causing these tests to pass. This commit changes the tests to use `assert.ok()` and removes redundant indices. PR-URL: #12040 Ref: #3567 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Some of the tests for `buffer.includes()` functionality introduced in #3567 have been broken in a way that caused them to always pass regardless of the result of the tested method. This behavior was caused by two reasons: * These tests were written as though `buffer.includes()` was supposed to return the same value that `buffer.indexOf()` does, i.e., used indices or -1 as expected return values instead of true and false. * `assert()` was used as the assertion function to do that instead of `assert.strictEqual()`. Thus `assert()` was called with a non-zero number as the first argument effectively causing these tests to pass. This commit changes the tests to use `assert.ok()` and removes redundant indices. PR-URL: #12040 Ref: #3567 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Some of the tests for `buffer.includes()` functionality introduced in #3567 have been broken in a way that caused them to always pass regardless of the result of the tested method. This behavior was caused by two reasons: * These tests were written as though `buffer.includes()` was supposed to return the same value that `buffer.indexOf()` does, i.e., used indices or -1 as expected return values instead of true and false. * `assert()` was used as the assertion function to do that instead of `assert.strictEqual()`. Thus `assert()` was called with a non-zero number as the first argument effectively causing these tests to pass. This commit changes the tests to use `assert.ok()` and removes redundant indices. PR-URL: #12040 Ref: #3567 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Some of the tests for `buffer.includes()` functionality introduced in #3567 have been broken in a way that caused them to always pass regardless of the result of the tested method. This behavior was caused by two reasons: * These tests were written as though `buffer.includes()` was supposed to return the same value that `buffer.indexOf()` does, i.e., used indices or -1 as expected return values instead of true and false. * `assert()` was used as the assertion function to do that instead of `assert.strictEqual()`. Thus `assert()` was called with a non-zero number as the first argument effectively causing these tests to pass. This commit changes the tests to use `assert.ok()` and removes redundant indices. PR-URL: #12040 Ref: #3567 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Some of the tests for `buffer.includes()` functionality introduced in nodejs/node#3567 have been broken in a way that caused them to always pass regardless of the result of the tested method. This behavior was caused by two reasons: * These tests were written as though `buffer.includes()` was supposed to return the same value that `buffer.indexOf()` does, i.e., used indices or -1 as expected return values instead of true and false. * `assert()` was used as the assertion function to do that instead of `assert.strictEqual()`. Thus `assert()` was called with a non-zero number as the first argument effectively causing these tests to pass. This commit changes the tests to use `assert.ok()` and removes redundant indices. PR-URL: nodejs/node#12040 Ref: nodejs/node#3567 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Simply adds Buffer.prototype.includes by wrapping an indexOf and
performing a strict equals check to -1.
The includes method takes the search value, byteOffset, and
encoding as arguments.
The test is a modified version of the indexOf test.
Addresses #3552.