FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

buffer: add Buffer compare by offset by jasnell · Pull Request #5880 · nodejs/node · GitHub

/ node Public

buffer: add Buffer compare by offset - #5880

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:buffer-compare-offset
ConversationCommits (1)ChecksFiles changed
Closed

buffer: add Buffer compare by offset#5880
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:buffer-compare-offset

Conversation

jasnell commented Mar 23, 2016

Copy link
Copy Markdown
Member

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

buffer

Description of change

Adds additional offset, length and thisOffset arguments
to Buffer.prototype.compare to allow comparison of sub-ranges
of two Buffers without requiring Buffer.prototype.slice()

Fixes: #521
/cc @trevnorris @rootslab @jorangreef

jasnell added 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. labels Mar 23, 2016
mscdex reviewed Mar 24, 2016
View reviewed changes
Comment thread doc/api/buffer.markdown Outdated

mscdex Mar 24, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Perhaps the order should be tweaked a bit here and have length and thisOffset swapped to match more closely with buffer.copy() which also deals with two buffers?

I also think offset might be better renamed to something like otherOffset.

trevnorris Mar 24, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Possibly follow suit of copy() since it follows similar parameters.

jasnell commented Mar 24, 2016

Copy link
Copy Markdown
Member Author

@trevnorris @mscdex ... updated! PTAL

The signature has been changed to: buf.compare(other[, otherStart[, thisStart[, otherEnd[, thisEnd]]]])

mscdex commented Mar 24, 2016

Copy link
Copy Markdown
Contributor

Couldn't we just have one end value, thisEnd, or just keep length instead of the end parameters? If not, I would prefer seeing buf.compare(other[, otherStart[, otherEnd[, thisStart[, thisEnd]]]]).

jasnell commented Mar 24, 2016

Copy link
Copy Markdown
Member Author

Personally I'd prefer to keep length. I changed it to this to align with the signature for buf.copy()

jasnell commented Mar 24, 2016

Copy link
Copy Markdown
Member Author

That said, having it as just length limits you to always comparing equal length ranges which is not always desirable.

jasnell commented Mar 24, 2016

Copy link
Copy Markdown
Member Author

@mscdex ... updated ... I swapped the order of the arguments but kept separate otherEnd and thisEnd.

The signature has been changed to: buf.compare(other[, otherStart[, otherEnd[, thisStart[, thisEnd]]]])

jasnell commented Mar 24, 2016

Copy link
Copy Markdown
Member Author

Currently, thisEnd defaults to this.byteLength, however, we could make it so that if thisEnd is not specified, it defaults to Math.min(this.byteLength, thisStart + (otherEnd - otherStart)))

jasnell commented Mar 24, 2016

Copy link
Copy Markdown
Member Author

doing so causes surprising behavior tho:

const a = Buffer.from([1,2,3,4,5,6,7,8,9]);
const b = Buffer.from([5,6,7,8,9,1,2,3,4]);
console.log(a.compare(b, 5, 9));
  // Prints 0

mscdex commented Mar 24, 2016

Copy link
Copy Markdown
Contributor

I don't think it's that much of a surprising behavior considering that the default for sourceStart for .copy() is also 0.

jasnell commented Mar 24, 2016

Copy link
Copy Markdown
Member Author

the surprising behavior is that the subset of b is not compared to the full content of a. If I did Buffer.compare(a, b.slice(5, 9)) I would get a different result.

trevnorris reviewed Mar 25, 2016
View reviewed changes
Comment thread doc/api/buffer.markdown Outdated

trevnorris Mar 25, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can we use source/target to mirror copy()? Think it will have less mental impact.

jasnell Mar 25, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

we could but source/target make a lot more sense in copy than they do in compare.

trevnorris commented Mar 25, 2016

Copy link
Copy Markdown
Contributor

@jasnell One documentation comment, but implementation LGTM.

jasnell force-pushed the buffer-compare-offset branch from 904cdf7 to 11f429f Compare March 26, 2016 04:07

jasnell commented Mar 26, 2016

Copy link
Copy Markdown
Member Author

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

jasnell commented Mar 26, 2016

Copy link
Copy Markdown
Member Author

@mscdex ... any further thoughts on this one?

mscdex reviewed Mar 26, 2016
View reviewed changes
Comment thread benchmark/buffers/buffer-compare-offset.js Outdated

mscdex Mar 26, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What about moving the bench.start()/bench.end() to be closer to the actual loop in the respective functions?

mscdex commented Mar 26, 2016

Copy link
Copy Markdown
Contributor

@jasnell Just a few more comments/questions.

jasnell force-pushed the buffer-compare-offset branch from 11f429f to cd114a2 Compare March 27, 2016 20:49

jasnell commented Mar 27, 2016

Copy link
Copy Markdown
Member Author

@trevnorris @mscdex ... updated and rebased. PTAL!

jasnell commented Apr 7, 2016

Copy link
Copy Markdown
Member Author

@trevnorris ... PTAL!

trevnorris reviewed Apr 8, 2016
View reviewed changes
Comment thread src/node_buffer.cc Outdated

trevnorris Apr 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

These should have been handled in JS. Instead can they all be turned into CHECKs? e.g.

CHECK(target_start >= target_end && source_start >= source_end);
CHECK_GE(source_start >= source_end);
CHECK_GE(target_start >= target_end);

Since if one of these is hit then it would indicate a bug on the JS side.

trevnorris commented Apr 8, 2016

Copy link
Copy Markdown
Contributor

One comment, but LGTM otherwise.

buffer: add Buffer.prototype.compare by offset
1e6a4c4 
Adds additional `targetStart`, `targetEnd`, `sourceStart,
and `sourceEnd` arguments to `Buffer.prototype.compare`
to allow comparison of sub-ranges of two Buffers without
requiring Buffer.prototype.slice()

Fixes: nodejs#521
jasnell force-pushed the buffer-compare-offset branch from 168f7ad to 1e6a4c4 Compare April 9, 2016 00:28

jasnell commented Apr 9, 2016

Copy link
Copy Markdown
Member Author

@trevnorris ... updated! squashed the commits. New CI: https://ci.nodejs.org/job/node-test-pull-request/2227/

trevnorris commented Apr 9, 2016

Copy link
Copy Markdown
Contributor

If CI is happy LGTM

jasnell commented Apr 9, 2016

Copy link
Copy Markdown
Member Author

Failures in CI are unrelated.

jasnell added a commit that referenced this pull request Apr 9, 2016
buffer: add Buffer.prototype.compare by offset
a246689 
Adds additional `targetStart`, `targetEnd`, `sourceStart,
and `sourceEnd` arguments to `Buffer.prototype.compare`
to allow comparison of sub-ranges of two Buffers without
requiring Buffer.prototype.slice()

Fixes: #521
PR-URL: #5880
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

jasnell commented Apr 9, 2016

Copy link
Copy Markdown
Member Author

Landed in a246689. Thank you!

jasnell closed this Apr 9, 2016

jorangreef commented Apr 9, 2016

Copy link
Copy Markdown
Contributor

Thanks @jasnell and @trevnorris.

MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
buffer: add Buffer.prototype.compare by offset
473f086 
Adds additional `targetStart`, `targetEnd`, `sourceStart,
and `sourceEnd` arguments to `Buffer.prototype.compare`
to allow comparison of sub-ranges of two Buffers without
requiring Buffer.prototype.slice()

Fixes: #521
PR-URL: #5880
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
2016-04-20, Version 5.11.0 (Stable) Release
e43735b 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069
This was referenced Apr 21, 2016
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
buffer: add Buffer.prototype.compare by offset
24ec701 
Adds additional `targetStart`, `targetEnd`, `sourceStart,
and `sourceEnd` arguments to `Buffer.prototype.compare`
to allow comparison of sub-ranges of two Buffers without
requiring Buffer.prototype.slice()

Fixes: #521
PR-URL: #5880
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
2016-04-21, Version 5.11.0 (Stable) Release
8dc1d5e 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behavior was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
2016-04-20, Version 5.11.0 (Stable) Release
0c5ab5c 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069

PR-URL: #6322
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
2016-04-20, Version 5.11.0 (Stable) Release
1fa8fce 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069

PR-URL: #6322
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
2016-04-20, Version 5.11.0 (Stable) Release
002d57b 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) nodejs#5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) nodejs#6279
  * update ESLint to 2.7.0
    (silverwind) nodejs#6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) nodejs#6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) nodejs#6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) nodejs#6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) nodejs#6090

src:
  * add SIGINFO to supported signals
    (James Reggio) nodejs#6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) nodejs#6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) nodejs#6069

PR-URL: nodejs#6322
jasnell added a commit that referenced this pull request Apr 26, 2016
buffer: add Buffer.prototype.compare by offset
6832078 
Adds additional `targetStart`, `targetEnd`, `sourceStart,
and `sourceEnd` arguments to `Buffer.prototype.compare`
to allow comparison of sub-ranges of two Buffers without
requiring Buffer.prototype.slice()

Fixes: #521
PR-URL: #5880
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
2016-04-20, Version 5.11.0 (Stable) Release
c764c44 
Buffer:
  * Buffer.prototype.compare can now compare sub-ranges of two Buffers
   (James M Snell) #5880

deps:
  * update to http-parser 2.7.0
    (Fedor Indutny) #6279
  * update ESLint to 2.7.0
    (silverwind) #6132

net:
  * adds support for passing DNS lookup hints to createConnection()
    (Colin Ihrig) #6000

node:
  * Make the builtin libraries available for the --eval and --print
    CLI options
    (Anna Henningsen) #6207

npm:
  * upgrade npm to 3.8.6
    (Kat Marchán) #6153

repl:
  * Pressing enter in the repl will repeat the last command by default
    if no input has been received. This behaviour was in node
    previously and was not removed intentionally.
    (Rich Trott) #6090

src:
  * add SIGINFO to supported signals
    (James Reggio) #6093

streams:
  * Fix a regression that caused by net streams requesting multiple
    chunks synchronously when combined with cork/uncork
    (Matteo Collina) #6164

zlib:
  * The flushing flag is now configurable allowing for decompression
    of partial data
    (Anna Henningsen) #6069

PR-URL: #6322
This file contains hidden or 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
Show hidden characters
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.

Buffer - feature request: Buffer.compare with offsets.

4 participants

Back | FazBrowse Home | New Git URL