src,test: add int64 test and fix JSRT #496

kfarnung · 2018-03-12T20:55:03Z

Fixed a couple build warnings as well.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

kfarnung · 2018-03-12T20:56:04Z

CI: ~~https://ci.nodejs.org/job/chakracore-test-pull-request/218/~~
https://ci.nodejs.org/job/chakracore-test-pull-request/220/

MSLaguana · 2018-03-12T20:57:40Z

src/node_api_jsrt.cc

@@ -443,7 +419,7 @@ static napi_status SetErrorCode(JsValueRef error,
    CHECK_JSRT(JsHasProperty(error, namePropId, &hasProp));

    JsValueRef nameValue = JS_INVALID_REFERENCE;
-    std::array<JsValueRef, 2> args = { nameArray, JS_INVALID_REFERENCE };
+    std::array<JsValueRef, 2> args = {{ nameArray, JS_INVALID_REFERENCE }};


Would you mind explaining this? was the initializer list previously hitting a different constructor?

clang (on macOS 10.13.4) specifically generated a warning that these braces should be doubled up. Looking around it seems like most people suggest ignoring that warning and I see mention that clang 6.0 is specifically disabling the warning by default.

https://stackoverflow.com/questions/13905200/is-it-wise-to-ignore-gcc-clangs-wmissing-braces-warning

Given that I'll undo this change.

boingoing

Looks alright. Plans to upsteam the unit tests? I assume they're compatible with v8.

kfarnung · 2018-03-13T00:37:22Z

@boingoing I plan to open an upstream PR once this lands.

Fixed a couple build warnings as well. PR-URL: nodejs#496 Reviewed-By: Taylor Woll <[email protected]> Reviewed-By: Jimmy Thomson <[email protected]>

Fixed a couple build warnings as well. PR-URL: nodejs#496 Reviewed-By: Taylor Woll <[email protected]> Reviewed-By: Jimmy Thomson <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

Fixed a couple build warnings as well. PR-URL: #496 Reviewed-By: Taylor Woll <[email protected]> Reviewed-By: Jimmy Thomson <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

Added some simple tests to verify that the int64 API is correctly handling numbers greater than 32-bits. This is a basic test, but verifies that an implementer hasn't truncated back to 32-bits. Refs: nodejs/node-chakracore#496 PR-URL: nodejs#19309 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]>

Added some simple tests to verify that the int64 API is correctly handling numbers greater than 32-bits. This is a basic test, but verifies that an implementer hasn't truncated back to 32-bits. Refs: nodejs/node-chakracore#496 PR-URL: #19309 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]>

Added some simple tests to verify that the int64 API is correctly handling numbers greater than 32-bits. This is a basic test, but verifies that an implementer hasn't truncated back to 32-bits. Refs: nodejs/node-chakracore#496 PR-URL: nodejs#19309 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]>

Added some simple tests to verify that the int64 API is correctly handling numbers greater than 32-bits. This is a basic test, but verifies that an implementer hasn't truncated back to 32-bits. Refs: nodejs/node-chakracore#496 Backport-PR-URL: #19447 PR-URL: #19309 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]>

Added some simple tests to verify that the int64 API is correctly handling numbers greater than 32-bits. This is a basic test, but verifies that an implementer hasn't truncated back to 32-bits. Refs: nodejs/node-chakracore#496 Backport-PR-URL: #19265 PR-URL: #19309 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]>

@rhuanjl

[MERGE #5583 @rhuanjl] Add end of file new line check fix #496 Merge pull request #5583 from rhuanjl:endOfFile Per issue #496 augment the existing line ending check to also check to ensure that each file ends in a newline. Note, per logic in check_eol.sh this test is run only on: 1. Files edited in the current PR 2. Files that are not JS test files OR any cmd, baseline, wasm, vcxproj or sln files Also note: 1. As this runs only on files edited din current PR it won't catch any historic mistakes 2. It doesn't check JS test files - this seemed odd to me but chose to leave it CC @dilijev Reviewed-By: chakrabot <[email protected]>

kfarnung self-assigned this Mar 12, 2018

kfarnung requested review from digitalinfinity, boingoing and MSLaguana March 12, 2018 20:55

MSLaguana reviewed Mar 12, 2018

View reviewed changes

boingoing approved these changes Mar 12, 2018

View reviewed changes

MSLaguana approved these changes Mar 12, 2018

View reviewed changes

digitalinfinity approved these changes Mar 13, 2018

View reviewed changes

kfarnung force-pushed the jsrtint64 branch from f330ae3 to 27cdd94 Compare March 13, 2018 00:41

n-api,test: add int64 test and fix JSRT

2b4f308

Fixed a couple build warnings as well. PR-URL: nodejs#496 Reviewed-By: Taylor Woll <[email protected]> Reviewed-By: Jimmy Thomson <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

kfarnung force-pushed the jsrtint64 branch from 27cdd94 to 2b4f308 Compare March 13, 2018 00:42

kfarnung merged commit 2b4f308 into nodejs:master Mar 13, 2018

kfarnung deleted the jsrtint64 branch March 13, 2018 00:43

kfarnung mentioned this pull request Mar 13, 2018

n-api,test: add int64 bounds tests nodejs/node#19309

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src,test: add int64 test and fix JSRT #496

src,test: add int64 test and fix JSRT #496

kfarnung commented Mar 12, 2018

kfarnung commented Mar 12, 2018 •

edited

Loading

MSLaguana Mar 12, 2018

kfarnung Mar 12, 2018

boingoing left a comment

kfarnung commented Mar 13, 2018

src,test: add int64 test and fix JSRT #496

src,test: add int64 test and fix JSRT #496

Conversation

kfarnung commented Mar 12, 2018

Checklist

kfarnung commented Mar 12, 2018 • edited Loading

MSLaguana Mar 12, 2018

Choose a reason for hiding this comment

kfarnung Mar 12, 2018

Choose a reason for hiding this comment

boingoing left a comment

Choose a reason for hiding this comment

kfarnung commented Mar 13, 2018

kfarnung commented Mar 12, 2018 •

edited

Loading