buffer: Added Buffer#includes() to keep parity with TypedArray. by umamialex · Pull Request #3567 · nodejs/node

umamialex · 2015-10-28T16:43:13Z

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.

Fishrock123 · 2015-10-28T16:49:18Z

cc @trevnorris

evanlucas · 2015-10-28T16:57:27Z

test/parallel/test-buffer-includes.js

const instead of var please

umamialex · 2015-10-28T17:09:30Z

Switched var to const where possible.

bnoordhuis · 2015-10-28T17:53:32Z

Isn't this a little premature? TypedArray#includes() is still behind a flag in the V8 we ship, isn't it?

trevnorris · 2015-10-28T17:56:21Z

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

trevnorris · 2015-10-28T17:57:41Z

@suitupalex Excellent job on all the tests. LGTM if CI is happy.

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

evanlucas · 2015-10-28T18:20:57Z

Mind adding docs for it?

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.

umamialex · 2015-10-28T18:40:06Z

@trevnorris thanks! Glad to be able to contribute.

@evanlucas absolutely. Just reupped with documentation.

evanlucas · 2015-10-28T21:25:58Z

CI looks good. Those failures look unrelated. It looks like the PPC machines are having some trouble though? /cc @nodejs/build

evanlucas · 2015-10-28T21:34:03Z

LGTM

rvagg · 2015-10-28T23:02:46Z

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.

rvagg · 2015-10-28T23:03:36Z

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

trevnorris · 2015-10-29T06:11:41Z

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

domenic · 2015-10-30T15:33:58Z

includes was specifically designed not to suck so I'd at least like to see some benchmarks before creating an overriden version...

trevnorris · 2015-10-30T20:31:19Z

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

domenic · 2015-10-30T21:23:05Z

Why? That isn't what "includes" means in ES.

targos · 2015-10-30T21:30:35Z

It would be similar to String#includes:

new Buffer('abcd').includes('bc') <=> 'abcd'.includes('bc')

Fishrock123 · 2015-10-30T22:52:23Z

@domenic Keep in mind this isn't TypedArray.includes -- this is for buffer, which acts more like a string in some cases.

Fishrock123 · 2015-11-03T16:07:45Z

Is anything blocking this?

trevnorris · 2015-11-03T18:06:43Z

Nothing from me.

Fishrock123 · 2015-11-05T16:01:15Z

LGTM

jasnell · 2015-11-05T18:04:29Z

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

trevnorris · 2015-12-09T23:24:10Z

@Fishrock123 Follow up, can this be merged?

Fishrock123 · 2015-12-10T17:32:57Z

@trevnorris is the agreement to ship in master but not land it in a stable until TypedArray adds it?

trevnorris · 2015-12-10T20:32:13Z

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

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>

trevnorris · 2015-12-10T20:48:08Z

Thanks much! Landed in 67e1819.

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

Fishrock123 added the buffer Issues and PRs related to the buffer subsystem. label Oct 28, 2015

Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 28, 2015

evanlucas reviewed Oct 28, 2015
View reviewed changes

test/parallel/test-buffer-includes.js Outdated

Copy link

Contributor

evanlucas Oct 28, 2015

Choose a reason for hiding this comment

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

const instead of var please

Fishrock123 mentioned this pull request Oct 28, 2015

buffer: Add Buffer#includes() #3568

Closed

Fishrock123 mentioned this pull request Dec 9, 2015

Add Buffer#includes() #3552

Closed

trevnorris closed this Dec 10, 2015

Fishrock123 mentioned this pull request Dec 15, 2015

v5.3.0 Release Proposal (Stable) #4281

Closed

aqrln mentioned this pull request Mar 25, 2017

test: fix broken tests in test-buffer-includes #12040

Closed

2 tasks

Uh oh!

Conversation

umamialex commented Oct 28, 2015

Uh oh!

Fishrock123 commented Oct 28, 2015

Uh oh!

evanlucas Oct 28, 2015

Choose a reason for hiding this comment

Uh oh!

umamialex commented Oct 28, 2015

Uh oh!

bnoordhuis commented Oct 28, 2015

Uh oh!

trevnorris commented Oct 28, 2015

Uh oh!

trevnorris commented Oct 28, 2015

Uh oh!

evanlucas commented Oct 28, 2015

Uh oh!

umamialex commented Oct 28, 2015

Uh oh!

evanlucas commented Oct 28, 2015

Uh oh!

evanlucas commented Oct 28, 2015

Uh oh!

rvagg commented Oct 28, 2015

Uh oh!

rvagg commented Oct 28, 2015

Uh oh!

trevnorris commented Oct 29, 2015

Uh oh!

domenic commented Oct 30, 2015

Uh oh!

trevnorris commented Oct 30, 2015

Uh oh!

domenic commented Oct 30, 2015

Uh oh!

targos commented Oct 30, 2015

Uh oh!

Fishrock123 commented Oct 30, 2015

Uh oh!

Fishrock123 commented Nov 3, 2015

Uh oh!

trevnorris commented Nov 3, 2015

Uh oh!

Fishrock123 commented Nov 5, 2015

Uh oh!

jasnell commented Nov 5, 2015

Uh oh!

trevnorris commented Dec 9, 2015

Uh oh!

Fishrock123 commented Dec 10, 2015

Uh oh!

trevnorris commented Dec 10, 2015

Uh oh!

trevnorris commented Dec 10, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants