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

n-api: re-write test_make_callback #19448

Conversation

gabrielschulhof
Copy link
Contributor

This re-writes the test in C by dropping std::vector<napi_value> in
favour of a C99 variable length array, and by dropping the anonymous
namespace in favour of static function declarations.

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

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Mar 19, 2018
@lpinca
Copy link
Member

lpinca commented Mar 20, 2018

@gabrielschulhof gabrielschulhof force-pushed the n-api-move-make-callback-test-to-c branch from c7d052c to 8e607b8 Compare March 20, 2018 13:57
@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof gabrielschulhof force-pushed the n-api-move-make-callback-test-to-c branch from 8e607b8 to 6ba6588 Compare March 20, 2018 15:35
@gabrielschulhof
Copy link
Contributor Author

Addressing Windows compilation issue (hopefully): https://ci.nodejs.org/job/node-test-pull-request/13770/

This re-writes the test in C by dropping std::vector<napi_value> in
favour of a C99 variable length array, and by dropping the anonymous
namespace in favour of static function declarations.
@gabrielschulhof gabrielschulhof force-pushed the n-api-move-make-callback-test-to-c branch from 6ba6588 to ab8328a Compare March 20, 2018 16:57
@gabrielschulhof
Copy link
Contributor Author

Another attempt to address the compilation issue: https://ci.nodejs.org/job/node-test-pull-request/13774/

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM provided CI passes

@gabrielschulhof
Copy link
Contributor Author

The remaining errors are unrelated.

@gabrielschulhof
Copy link
Contributor Author

Landed in be47094.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Mar 21, 2018
This re-writes the test in C by dropping std::vector<napi_value> in
favour of a C99 variable length array, and by dropping the anonymous
namespace in favour of static function declarations.

PR-URL: nodejs#19448
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gabrielschulhof gabrielschulhof deleted the n-api-move-make-callback-test-to-c branch March 21, 2018 23:26
targos pushed a commit that referenced this pull request Mar 24, 2018
This re-writes the test in C by dropping std::vector<napi_value> in
favour of a C99 variable length array, and by dropping the anonymous
namespace in favour of static function declarations.

PR-URL: #19448
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
This re-writes the test in C by dropping std::vector<napi_value> in
favour of a C99 variable length array, and by dropping the anonymous
namespace in favour of static function declarations.

PR-URL: nodejs#19448
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
This re-writes the test in C by dropping std::vector<napi_value> in
favour of a C99 variable length array, and by dropping the anonymous
namespace in favour of static function declarations.

PR-URL: nodejs#19448
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
This re-writes the test in C by dropping std::vector<napi_value> in
favour of a C99 variable length array, and by dropping the anonymous
namespace in favour of static function declarations.

Backport-PR-URL: #19447
PR-URL: #19448
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
MylesBorins pushed a commit that referenced this pull request May 1, 2018
This re-writes the test in C by dropping std::vector<napi_value> in
favour of a C99 variable length array, and by dropping the anonymous
namespace in favour of static function declarations.

Backport-PR-URL: #19265
PR-URL: #19448
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants