src: add include to js_native_api_v8.cc for standalone compile #24498

bghgary · 2018-11-20T00:39:53Z

Include algorithm header in js_native_api_v8.cc since std::min
requires it.
Add comments to js_native_api_v8_internals.h for NAPI_VERSION

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

CC @gabrielschulhof and @mhdawson

refack · 2018-11-20T02:05:36Z

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

mhdawson · 2018-11-20T15:25:23Z

Open issue for failure on AIX which seems unrelated - #24519

mhdawson · 2018-11-20T15:27:44Z

Failure on ubuntu also looks unrelated, pre-existing issue: #24403

mhdawson · 2018-11-20T15:29:01Z

Resumed build here: https://ci.nodejs.org/job/node-test-pull-request/18807/

joyeecheung

(This makes me wonder if we can just put the js_native_api files into a a folder under src..)

gabrielschulhof · 2018-11-21T17:49:14Z

@bghgary this results in the following warning:

In file included from ../src/js_native_api_v8_internals.h:13:0,
                 from ../src/js_native_api_v8.h:6,
                 from ../src/js_native_api_v8.cc:5:
../src/node_version.h:120:0: warning: "NAPI_VERSION" redefined
 #define NAPI_VERSION  3
 
In file included from ../src/js_native_api_v8.h:5:0,
                 from ../src/js_native_api_v8.cc:5:
../src/js_native_api.h:11:0: note: this is the location of the previous definition
 #define NAPI_VERSION 2147483647

gabrielschulhof

I'd like to find a way to address this warning before this lands.

gabrielschulhof · 2018-11-21T17:52:18Z

@bghgary which code in js_native_api_v8.h depends on js_native_api.h? Why is js_native_api_types.h insufficient?

src/js_native_api_v8.h

bghgary · 2018-11-21T18:25:46Z

which code in js_native_api_v8.h depends on js_native_api.h? Why is js_native_api_types.h insufficient?

Arg, I made a mistake and was on an older commit which didn't have the final changes. js_native_api_types.h is sufficient. This wasn't included originally. The <algorithm> header is still necessary though.

I'll update this with just the change.

gabrielschulhof · 2018-11-21T18:42:47Z

@bghgary Awesome! including js_native_api_types.h is indeed the least-impact change. Don't worry about the comments, either. I can make a second PR.

gabrielschulhof · 2018-11-21T18:43:56Z

@bghgary Oh, NM 🙂 I just saw that you'd add the comments too. Thanks!

- Include algorithm header in js_native_api_v8.cc since std::min requires it. - Add comments to js_native_api_v8_internals.h for NAPI_VERSION

bghgary · 2018-11-21T18:56:35Z

Updated commit, changed title, changed PR description.

bghgary · 2018-11-21T22:47:12Z

There is an error in the CI that I don't think is related to my changes. Can anyone take a look?

bghgary · 2018-11-21T22:47:59Z

It says "API rate limit exceeded for 35.224.112.202"

refack · 2018-11-21T22:51:53Z

It says "API rate limit exceeded for 35.224.112.202"

That job only validates the format of the commit message. So errors there are non blocking.
Full CI: https://ci.nodejs.org/job/node-test-pull-request/18858/

gabrielschulhof · 2018-11-22T00:39:53Z

Landed in d1a55d3.

- Include algorithm header in js_native_api_v8.cc since std::min requires it. - Add comments to js_native_api_v8_internals.h for NAPI_VERSION PR-URL: #24498 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

refack · 2018-11-22T00:52:36Z

@bghgary 🎉 Congratulations on getting promoted by GitHub from

to

Hope you had a welcoming experience and that we'll see you contributing more in the future.

bghgary · 2018-11-22T02:00:42Z

Thanks! I have more changes coming. No worries on that. 😅

- Include algorithm header in js_native_api_v8.cc since std::min requires it. - Add comments to js_native_api_v8_internals.h for NAPI_VERSION PR-URL: #24498 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

- Include algorithm header in js_native_api_v8.cc since std::min requires it. - Add comments to js_native_api_v8_internals.h for NAPI_VERSION PR-URL: nodejs#24498 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 20, 2018

bghgary mentioned this pull request Nov 20, 2018

src: factor out Node.js-agnostic N-APIs #23786

Closed

refack approved these changes Nov 20, 2018

View reviewed changes

refack added the node-api Issues and PRs related to the Node-API. label Nov 20, 2018

mhdawson closed this Nov 20, 2018

mhdawson reopened this Nov 20, 2018

mhdawson closed this Nov 20, 2018

mhdawson reopened this Nov 20, 2018

joyeecheung approved these changes Nov 20, 2018

View reviewed changes

cjihrig approved these changes Nov 21, 2018

View reviewed changes

jasnell approved these changes Nov 21, 2018

View reviewed changes

bghgary mentioned this pull request Nov 21, 2018

test: partition N-API tests #24557

Closed

4 tasks

gabrielschulhof requested changes Nov 21, 2018

View reviewed changes

gabrielschulhof reviewed Nov 21, 2018

View reviewed changes

src/js_native_api_v8.h Outdated Show resolved Hide resolved

src: add include to js_native_api_v8.cc for standalone compile

6fff71c

- Include algorithm header in js_native_api_v8.cc since std::min requires it. - Add comments to js_native_api_v8_internals.h for NAPI_VERSION

bghgary force-pushed the js-native-api-fixes branch from b94c62f to 6fff71c Compare November 21, 2018 18:54

gabrielschulhof approved these changes Nov 21, 2018

View reviewed changes

bghgary changed the title ~~src: fix includes in js_native_api_v8.* so that they compile standalone~~ src: add include to js_native_api_v8.cc for standalone compile Nov 21, 2018

refack mentioned this pull request Nov 21, 2018

build: lint commit message in separate Travis job #24254

Closed

1 task

refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2018

gabrielschulhof closed this Nov 22, 2018

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

codebytere added the dont-land-on-v10.x label Jan 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: add include to js_native_api_v8.cc for standalone compile #24498

src: add include to js_native_api_v8.cc for standalone compile #24498

bghgary commented Nov 20, 2018 •

edited

Loading

refack commented Nov 20, 2018

mhdawson commented Nov 20, 2018

mhdawson commented Nov 20, 2018

mhdawson commented Nov 20, 2018

joyeecheung left a comment

gabrielschulhof commented Nov 21, 2018

gabrielschulhof left a comment •

edited

Loading

gabrielschulhof commented Nov 21, 2018

bghgary commented Nov 21, 2018 •

edited

Loading

gabrielschulhof commented Nov 21, 2018

gabrielschulhof commented Nov 21, 2018

bghgary commented Nov 21, 2018

bghgary commented Nov 21, 2018

bghgary commented Nov 21, 2018

refack commented Nov 21, 2018

gabrielschulhof commented Nov 22, 2018

refack commented Nov 22, 2018

bghgary commented Nov 22, 2018

src: add include to js_native_api_v8.cc for standalone compile #24498

src: add include to js_native_api_v8.cc for standalone compile #24498

Conversation

bghgary commented Nov 20, 2018 • edited Loading

Checklist

refack commented Nov 20, 2018

mhdawson commented Nov 20, 2018

mhdawson commented Nov 20, 2018

mhdawson commented Nov 20, 2018

joyeecheung left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Nov 21, 2018

gabrielschulhof left a comment • edited Loading

Choose a reason for hiding this comment

gabrielschulhof commented Nov 21, 2018

bghgary commented Nov 21, 2018 • edited Loading

gabrielschulhof commented Nov 21, 2018

gabrielschulhof commented Nov 21, 2018

bghgary commented Nov 21, 2018

bghgary commented Nov 21, 2018

bghgary commented Nov 21, 2018

refack commented Nov 21, 2018

gabrielschulhof commented Nov 22, 2018

refack commented Nov 22, 2018

bghgary commented Nov 22, 2018

bghgary commented Nov 20, 2018 •

edited

Loading

gabrielschulhof left a comment •

edited

Loading

bghgary commented Nov 21, 2018 •

edited

Loading