Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
bghgary wants to merge 1 commit into from
Closed

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

bghgary wants to merge 1 commit into from

Conversation

@bghgary
Copy link
Contributor

@bghgarybghgary commented Nov 20, 2018•
edited
Loading

  • 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

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 20, 2018
@bghgarybghgary mentioned this pull request Nov 20, 2018
Closed
@refack
Copy link
Contributor

refack commented Nov 20, 2018

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

@refackrefack added the node-api Issues and PRs related to the Node-API. label Nov 20, 2018
@mhdawson
Copy link
Member

mhdawson commented Nov 20, 2018

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

@mhdawsonmhdawson reopened this Nov 20, 2018
@mhdawson
Copy link
Member

mhdawson commented Nov 20, 2018

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

@mhdawsonmhdawson reopened this Nov 20, 2018
@mhdawson
Copy link
Member

mhdawson commented Nov 20, 2018

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

joyeecheung
joyeecheung approved these changes Nov 20, 2018
View reviewed changes
Copy link
Member

@joyeecheungjoyeecheung left a comment

Choose a reason for hiding this comment

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

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

@bghgarybghgary mentioned this pull request Nov 21, 2018
Closed
4 tasks
@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Nov 21, 2018

@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
gabrielschulhof requested changes Nov 21, 2018
View reviewed changes
Copy link
Contributor

@gabrielschulhofgabrielschulhof left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Nov 21, 2018

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

gabrielschulhof
gabrielschulhof reviewed Nov 21, 2018
View reviewed changes
@bghgary
Copy link
ContributorAuthor

bghgary commented Nov 21, 2018•
edited
Loading

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
Copy link
Contributor

gabrielschulhof commented Nov 21, 2018

@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
Copy link
Contributor

gabrielschulhof commented Nov 21, 2018

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

@bghgary
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
@bghgarybghgary changed the title src: fix includes in js_native_api_v8.* so that they compile standalonesrc: add include to js_native_api_v8.cc for standalone compileNov 21, 2018
@bghgary
Copy link
ContributorAuthor

bghgary commented Nov 21, 2018

Updated commit, changed title, changed PR description.

@bghgary
Copy link
ContributorAuthor

bghgary commented Nov 21, 2018

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

@bghgary
Copy link
ContributorAuthor

bghgary commented Nov 21, 2018

It says "API rate limit exceeded for 35.224.112.202"

@refackrefack mentioned this pull request Nov 21, 2018
Closed
1 task
@refack
Copy link
Contributor

refack commented Nov 21, 2018

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/

@refackrefack 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
Copy link
Contributor

gabrielschulhof commented Nov 22, 2018

Landed in d1a55d3.

gabrielschulhof pushed a commit that referenced this pull request Nov 22, 2018
@bghgary
src: add include for standalone compile
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
Copy link
Contributor

refack commented Nov 22, 2018

@bghgary 🎉 Congratulations on getting promoted by GitHub from
image
to
image
Hope you had a welcoming experience and that we'll see you contributing more in the future.

@bghgary
Copy link
ContributorAuthor

bghgary commented Nov 22, 2018

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

targos pushed a commit that referenced this pull request Nov 24, 2018
@bghgary@targos
src: add include for standalone compile
b9a9610
- 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]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@bghgary@rvagg
src: add include for standalone compile
b9bd4e9
- 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]>
@BridgeARBridgeAR mentioned this pull request Dec 5, 2018
Merged
4 tasks
This was referenced Dec 7, 2018
Closed
Closed
Closed
Closed
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@bghgary@refack
src: add include for standalone compile
4057370
- 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]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@gabrielschulhofgabrielschulhofgabrielschulhof approved these changes

@cjihrigcjihrigcjihrig approved these changes

@joyeecheungjoyeecheungjoyeecheung approved these changes

+1 more reviewer

@refackrefackrefack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.node-apiIssues and PRs related to the Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@bghgary@refack@mhdawson@gabrielschulhof@jasnell@cjihrig@joyeecheung@codebytere@nodejs-github-bot