Skip to content
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: standardize array index check #6084

Merged
merged 1 commit into from
Apr 8, 2016

Conversation

trevnorris
Copy link
Contributor

Checklist

  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

buffer

Description of change

ParseArrayIndex() was requesting a Uint32Value(), but assigning it to an
in32_t. This caused slight differences in error message reported in edge
cases of argument parsing. Fixed by getting the IntegerValue() before
checking if the value is < 0. Added test of API that was affected.

R=@bnoordhuis
R=@jasnell

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

@trevnorris trevnorris added the buffer Issues and PRs related to the buffer subsystem. label Apr 6, 2016
@jasnell
Copy link
Member

jasnell commented Apr 7, 2016

LGTM

ParseArrayIndex() was requesting a Uint32Value(), but assigning it to an
in32_t. This caused slight differences in error message reported in edge
cases of argument parsing. Fixed by getting the IntegerValue() before
checking if the value is < 0. Added test of API that was affected.

PR-URL: nodejs#6084
Reviewed-By: James M Snell <jasnell@gmail.com>
@trevnorris trevnorris merged commit 9d94cc5 into nodejs:master Apr 8, 2016
@trevnorris
Copy link
Contributor Author

Thanks much! Landed in 9d94cc5.

@trevnorris trevnorris deleted the fix-parseindex branch April 8, 2016 17:02
MylesBorins pushed a commit that referenced this pull request Apr 19, 2016
ParseArrayIndex() was requesting a Uint32Value(), but assigning it to an
in32_t. This caused slight differences in error message reported in edge
cases of argument parsing. Fixed by getting the IntegerValue() before
checking if the value is < 0. Added test of API that was affected.

PR-URL: #6084
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
ParseArrayIndex() was requesting a Uint32Value(), but assigning it to an
in32_t. This caused slight differences in error message reported in edge
cases of argument parsing. Fixed by getting the IntegerValue() before
checking if the value is < 0. Added test of API that was affected.

PR-URL: #6084
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 20, 2016
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
ParseArrayIndex() was requesting a Uint32Value(), but assigning it to an
in32_t. This caused slight differences in error message reported in edge
cases of argument parsing. Fixed by getting the IntegerValue() before
checking if the value is < 0. Added test of API that was affected.

PR-URL: #6084
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Apr 21, 2016
jasnell pushed a commit that referenced this pull request Apr 26, 2016
ParseArrayIndex() was requesting a Uint32Value(), but assigning it to an
in32_t. This caused slight differences in error message reported in edge
cases of argument parsing. Fixed by getting the IntegerValue() before
checking if the value is < 0. Added test of API that was affected.

PR-URL: #6084
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

@trevnorris there are some failing tests on v4.x-staging with this change.

Here's the diff

++<<<<<<< 5faeb7ebf381445bc1a156669f983d6133659310
 +  int32_t tmp_i = arg->Int32Value();
++=======
+   int64_t tmp_i = arg->IntegerValue();
++>>>>>>> buffer: standardize array index check

Perhaps the change isn't necessary since the assigned value and return match up on the v4.x stream

@MylesBorins
Copy link
Contributor

ping @trevnorris should this be backported?

@trevnorris
Copy link
Contributor Author

@thealphanerd I wouldn't consider this something that's necessary to backport. Especially since it implicitly relies on several other parts working in a specific way.

@MylesBorins
Copy link
Contributor

thanks for the heads up. moving it to don't land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants