Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

n-api: add more int64_t tests #500

Merged
merged 1 commit into from
Mar 16, 2018
Merged

n-api: add more int64_t tests #500

merged 1 commit into from
Mar 16, 2018

Conversation

kfarnung
Copy link
Contributor

Also updated/added tests for Number and int32_t as well.

Updated N-API int64 behavior to check for all non-finite numbers
instead of just NaN.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@kfarnung kfarnung self-assigned this Mar 16, 2018
kfarnung
kfarnung commented Mar 16, 2018
View reviewed changes
test/addons-napi/test_number/test.js Outdated

// Test non-finite numbers
testInt64(Number.POSITIVE_INFINITY, 0);
testInt64(Number.NEGATIVE_INFINITY, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two will fail if ported upstream until std::isnan is replaced with !std::isinfinite

boingoing
boingoing reviewed Mar 16, 2018
View reviewed changes
test/addons-napi/test_number/test.js Outdated
// Test min/max double value
testInt64(Number.MIN_VALUE, 0);
testInt64(-Number.MIN_VALUE, 0);
////testInt64(Number.MAX_VALUE, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these ones work?

boingoing
boingoing approved these changes Mar 16, 2018
View reviewed changes
Copy link
Contributor

@boingoing boingoing left a comment

Choose a reason for hiding this comment

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

Looks fine to me. I'm not a big fan of checking-in commented-out tests unless there's a reason.

kfarnung
kfarnung commented Mar 16, 2018
View reviewed changes
test/addons-napi/test_number/test.js Outdated
testInt64(Number.MIN_VALUE, 0);
testInt64(-Number.MIN_VALUE, 0);
////testInt64(Number.MAX_VALUE, 0);
////testInt64(-Number.MAX_VALUE, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not clear what to do with these, we currently return 0x8000000000000000 when the cast fails (this appears to match the V8 behavior as well), but in reality this value just indicates that the operation failed. This appears to be the standard behavior for the cvttsd2si, but is platform-specific.

My preference would be to treat this as an error and return an error code indicating that the conversion failed.

@kfarnung
Copy link
Contributor Author

kfarnung commented Mar 16, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/chakracore-test-pull-request/222/
Linter Re-run: https://ci.nodejs.org/job/node-test-linter/17046/

@kfarnung
n-api: add more int64_t tests
8b958bf 
Also updated/added tests for `Number` and `int32_t` as well.

Updated N-API int64 behavior to check for all non-finite numbers
instead of just NaN.

PR-URL: nodejs#500
Reviewed-By: Taylor Woll <tawoll@ntdev.microsoft.com>
@kfarnung kfarnung merged commit 8b958bf into nodejs:master Mar 16, 2018
@kfarnung kfarnung mentioned this pull request Mar 17, 2018
Closed
4 tasks
@kfarnung kfarnung deleted the napi_int64 branch March 18, 2018 15:54
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Mar 28, 2018
@kfarnung
n-api: add more int64_t tests
c1dfb0e 
Also updated/added tests for `Number` and `int32_t` as well.

Updated N-API int64 behavior to check for all non-finite numbers
instead of just NaN.

PR-URL: nodejs#500
Reviewed-By: Taylor Woll <tawoll@ntdev.microsoft.com>
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Mar 28, 2018
@kfarnung
n-api: add more int64_t tests
c2f52f7 
Also updated/added tests for `Number` and `int32_t` as well.

Updated N-API int64 behavior to check for all non-finite numbers
instead of just NaN.

PR-URL: nodejs#500
Reviewed-By: Taylor Woll <tawoll@ntdev.microsoft.com>
kfarnung added a commit to nodejs/node that referenced this pull request Apr 8, 2018
@kfarnung
n-api: add more int64_t tests
c529168 
* Updated tests for `Number` and `int32_t`
* Added new tests for `int64_t`
* Updated N-API `int64_t` behavior to return zero for all non-finite
  numbers
* Clarified the documentation for these calls.

PR-URL: #19402
Refs: nodejs/node-chakracore#500
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
@kfarnung
n-api: add more int64_t tests
d73e802 
* Updated tests for `Number` and `int32_t`
* Added new tests for `int64_t`
* Updated N-API `int64_t` behavior to return zero for all non-finite
  numbers
* Clarified the documentation for these calls.

PR-URL: nodejs#19402
Refs: nodejs/node-chakracore#500
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Apr 12, 2018
@kfarnung @targos
n-api: add more int64_t tests
a63cdee 
* Updated tests for `Number` and `int32_t`
* Added new tests for `int64_t`
* Updated N-API `int64_t` behavior to return zero for all non-finite
  numbers
* Clarified the documentation for these calls.

PR-URL: #19402
Refs: nodejs/node-chakracore#500
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
@kfarnung
n-api: add more int64_t tests
f498a62 
* Updated tests for `Number` and `int32_t`
* Added new tests for `int64_t`
* Updated N-API `int64_t` behavior to return zero for all non-finite
  numbers
* Clarified the documentation for these calls.

PR-URL: nodejs#19402
Refs: nodejs/node-chakracore#500
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 16, 2018
@kfarnung @MylesBorins
n-api: add more int64_t tests
3aa5b7d 
* Updated tests for `Number` and `int32_t`
* Added new tests for `int64_t`
* Updated N-API `int64_t` behavior to return zero for all non-finite
  numbers
* Clarified the documentation for these calls.

Backport-PR-URL: #19447
PR-URL: #19402
Refs: nodejs/node-chakracore#500
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request May 1, 2018
@kfarnung @MylesBorins
n-api: add more int64_t tests
07a6770 
* Updated tests for `Number` and `int32_t`
* Added new tests for `int64_t`
* Updated N-API `int64_t` behavior to return zero for all non-finite
  numbers
* Clarified the documentation for these calls.

Backport-PR-URL: #19265
PR-URL: #19402
Refs: nodejs/node-chakracore#500
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@boingoing boingoing boingoing approved these changes

@digitalinfinity digitalinfinity Awaiting requested review from digitalinfinity

@MSLaguana MSLaguana Awaiting requested review from MSLaguana

Assignees

@kfarnung kfarnung

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kfarnung @boingoing