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

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

Merged
merged 1 commit into from
Mar 13, 2018
Merged

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

merged 1 commit into from
Mar 13, 2018

Conversation

kfarnung
Copy link
Contributor

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 kfarnung self-assigned this Mar 12, 2018
@kfarnung
Copy link
Contributor Author

kfarnung commented Mar 12, 2018 •
edited
Loading

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

MSLaguana
MSLaguana reviewed Mar 12, 2018
View reviewed changes
src/node_api_jsrt.cc Outdated
@@ -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 }};
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
boingoing approved these changes Mar 12, 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 alright. Plans to upsteam the unit tests? I assume they're compatible with v8.

@kfarnung
Copy link
Contributor Author

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

kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Mar 13, 2018
@kfarnung
n-api,test: add int64 test and fix JSRT
27cdd94 
Fixed a couple build warnings as well.

PR-URL: nodejs#496
Reviewed-By: Taylor Woll <tawoll@ntdev.microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
@kfarnung
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 <tawoll@ntdev.microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
@kfarnung kfarnung merged commit 2b4f308 into nodejs:master Mar 13, 2018
@kfarnung kfarnung deleted the jsrtint64 branch March 13, 2018 00:43
@kfarnung kfarnung mentioned this pull request Mar 13, 2018
Merged
3 tasks
kfarnung added a commit that referenced this pull request Mar 13, 2018
@kfarnung
n-api,test: add int64 test and fix JSRT
f5c5be3 
Fixed a couple build warnings as well.

PR-URL: #496
Reviewed-By: Taylor Woll <tawoll@ntdev.microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
kfarnung added a commit to kfarnung/node that referenced this pull request Mar 15, 2018
@kfarnung
n-api,test: add int64 bounds tests
5fb6f7f 
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 <cjihrig@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Mar 17, 2018
@kfarnung @targos
n-api,test: add int64 bounds tests
68eac4e 
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 <cjihrig@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Mar 20, 2018
@kfarnung @MylesBorins
n-api,test: add int64 bounds tests
38c7fa1 
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 <cjihrig@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Mar 20, 2018
@kfarnung @MylesBorins
n-api,test: add int64 bounds tests
0c9577e 
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 <cjihrig@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.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,test: add int64 bounds tests
4a0fc8a 
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 <cjihrig@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.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,test: add int64 bounds tests
fdd3f57 
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 <cjihrig@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.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,test: add int64 bounds tests
aa0fb77 
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 <cjihrig@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.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,test: add int64 bounds tests
584fadc 
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 <cjihrig@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
chakrabot pushed a commit that referenced this pull request Aug 28, 2018
@dilijev @chakrabot
deps: update ChakraCore to chakra-core/ChakraCore@6d96d5b986
ed5ee00 
[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 <chakrabot@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MSLaguana MSLaguana MSLaguana approved these changes

@boingoing boingoing boingoing approved these changes

@digitalinfinity digitalinfinity digitalinfinity approved these changes

Assignees

@kfarnung kfarnung

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kfarnung @digitalinfinity @boingoing @MSLaguana