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.

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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

bghgary
Copy link
Contributor

@bghgary bghgary commented Nov 20, 2018 •
edited

  • 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-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 20, 2018
@bghgary bghgary 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/

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

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

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

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

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

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

@joyeecheung joyeecheung 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..)

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

@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

@gabrielschulhof gabrielschulhof left a comment •
edited

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

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

@bghgary
Copy link
Contributor Author

bghgary commented Nov 21, 2018 •
edited

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

@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

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

bghgary commented Nov 21, 2018

Updated commit, changed title, changed PR description.

@bghgary
Copy link
Contributor Author

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
Contributor Author

bghgary commented Nov 21, 2018

It says "API rate limit exceeded for 35.224.112.202"

@refack refack 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/

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

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 <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@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
Contributor Author

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 <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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 <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BridgeAR BridgeAR 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 <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
author ready PRs 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-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@bghgary @refack @mhdawson @gabrielschulhof @jasnell @cjihrig @joyeecheung @codebytere @nodejs-github-bot