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.

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

buffer: fix indexOf for empty searches #13024

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@addaleax
Member

addaleax commented May 14, 2017

Make searches for empty subsequences return 0 for buffer.indexOf()
and buffer.length for buffer.lastIndexOf(), because those
are the indices of the first and last occurrence of an empty
subsequence, respectively.

Make searches for empty subsequences do exactly what String.prototype.indexOf() does.

Fixes: #13023

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

/cc @nodejs/buffer and because people might to consider this a semver-major change (I would not) @nodejs/ctc

CI: https://ci.nodejs.org/job/node-test-commit/9880/

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott May 14, 2017

Member

Q: semver-major?

Member

Trott commented May 14, 2017

Q: semver-major?

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott May 14, 2017

Member

Documentation update to include this info? MDN mentions the behavior in its docs for String.prototype.lastIndexOf() and String.prototype.indexOf() and we should probably follow suit.

Member

Trott commented May 14, 2017

Documentation update to include this info? MDN mentions the behavior in its docs for String.prototype.lastIndexOf() and String.prototype.indexOf() and we should probably follow suit.

Show outdated Hide outdated test/parallel/test-buffer-indexof.js
Show outdated Hide outdated test/parallel/test-buffer-indexof.js
Show outdated Hide outdated test/parallel/test-buffer-indexof.js
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax
Member

addaleax commented May 14, 2017

Show outdated Hide outdated doc/api/buffer.md
Show outdated Hide outdated doc/api/buffer.md
buffer: fix indexOf for empty searches
Make searches for empty subsequences do exactly what
`String.prototype.indexOf()` does.

Fixes: #13023
@Trott

Trott approved these changes May 14, 2017

LGTM to me if CI is green and doc nits are addressed. Not sure about the semver-ness here, but maybe CITGM run results would help make the case for patch vs. major?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 15, 2017

Member

Assuming previous Windows fails was infra. Rerunning: https://ci.nodejs.org/job/node-test-commit-windows-fanned/9084/

Member

refack commented May 15, 2017

Assuming previous Windows fails was infra. Rerunning: https://ci.nodejs.org/job/node-test-commit-windows-fanned/9084/

@refack

refack approved these changes May 15, 2017

// Valid positive offset.
return offset_i64;
} else if (needle_length == 0) {
// Out of buffer bounds, but empty needle: point to end of buffer.

This comment has been minimized.

@refack

refack May 15, 2017

Member

Suggestion:
Either ref - http://www.ecma-international.org/ecma-262/6.0/#sec-string.prototype.indexof
Or state Same semantics as String.prototype.indexof

@refack

refack May 15, 2017

Member

Suggestion:
Either ref - http://www.ecma-international.org/ecma-262/6.0/#sec-string.prototype.indexof
Or state Same semantics as String.prototype.indexof

This comment has been minimized.

@addaleax

addaleax May 15, 2017

Member

@refack Ack, added comments (at the GetReturnValue() sites)

@addaleax

addaleax May 15, 2017

Member

@refack Ack, added comments (at the GetReturnValue() sites)

This comment has been minimized.

@refack

refack May 15, 2017

Member

👍
I was feeling it missing here (you explain "what" but not "why")

@refack

refack May 15, 2017

Member

👍
I was feeling it missing here (you explain "what" but not "why")

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 18, 2017

Member

Landed in 28ddac2

Member

addaleax commented May 18, 2017

Landed in 28ddac2

@addaleax addaleax closed this May 18, 2017

@addaleax addaleax deleted the addaleax:buffer-indexof-emptystring branch May 18, 2017

Trott pushed a commit to Trott/io.js that referenced this pull request May 18, 2017

buffer: fix indexOf for empty searches
Make searches for empty subsequences do exactly what
`String.prototype.indexOf()` does.

Fixes: nodejs#13023
PR-URL: nodejs#13024
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

anchnk added a commit to anchnk/node that referenced this pull request May 19, 2017

buffer: fix indexOf for empty searches
Make searches for empty subsequences do exactly what
`String.prototype.indexOf()` does.

Fixes: nodejs#13023
PR-URL: nodejs#13024
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Jul 17, 2017

Member

@addaleax is this applicable for v6.x?

Member

MylesBorins commented Jul 17, 2017

@addaleax is this applicable for v6.x?

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Aug 14, 2017

Member

I'm going to opt to not land this on lts as it does change behavior of buffer. While it might be "more correct", I'd rather not pull the rug under people

/cc @addaleax @nodejs/ctc

Member

MylesBorins commented Aug 14, 2017

I'm going to opt to not land this on lts as it does change behavior of buffer. While it might be "more correct", I'd rather not pull the rug under people

/cc @addaleax @nodejs/ctc

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 14, 2017

Member

I’m okay with that.

Member

addaleax commented Aug 14, 2017

I’m okay with that.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Aug 16, 2017

Member

I'm ok as well.

Member

mcollina commented Aug 16, 2017

I'm ok as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment