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

bigint support #292

Merged
merged 1 commit into from
Sep 18, 2018
Merged

bigint support #292

merged 1 commit into from
Sep 18, 2018

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Jun 28, 2018

blocked until nodejs/node#21226 lands

@devsnek devsnek requested a review from addaleax June 28, 2018 15:09
@mhdawson
Copy link
Member

@devsnek. this would also need tests and doc to land.

@devsnek
Copy link
Member Author

devsnek commented Jun 29, 2018

@mhdawson working on it locally, mostly just uploaded now to get comments on the design

@devsnek
Copy link
Member Author

devsnek commented Jul 6, 2018

how should one add features that are under NAPI_EXPERIMENTAL in node_api? should we use the same preprocessor variable in node-addon-api?

@mhdawson
Copy link
Member

mhdawson commented Jul 9, 2018

@devsnek correct, our plans is to use the same NAPI_EXPERIMENTAL define in node-addon-api for any method that depends on an experimental feature.

@devsnek devsnek removed the blocked label Jul 12, 2018
@devsnek devsnek force-pushed the feature/bigint branch 5 times, most recently from baa8b4f to dd50036 Compare July 16, 2018 06:20
@devsnek
Copy link
Member Author

devsnek commented Jul 16, 2018

@addaleax @mhdawson tests and docs are in

@devsnek devsnek force-pushed the feature/bigint branch 3 times, most recently from ce7c06d to aea9def Compare July 16, 2018 06:50
NickNaso
NickNaso reviewed Jul 16, 2018
View reviewed changes
Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

Great work. Just some notes about documentation

doc/bigint.md Outdated
static BigInt New(Napi::Env env, uint64_t value);
```

- `[in] env`: The `napi_env` Environment
Copy link
Member

Choose a reason for hiding this comment

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

The input parameter is Napi::Env and in the rest of documentation we defined this parameter as "The environment in which to ..."

doc/bigint.md Outdated
const uint64_t* words);
```

- `[in] env`: The `napi_env` Environment
Copy link
Member

Choose a reason for hiding this comment

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

The parameter env is Napi::Env and in the rest of documentation we defined this parameter as "The environment in which to ..."

@devsnek
Copy link
Member Author

devsnek commented Jul 18, 2018

@NickNaso comments were addressed

NickNaso
NickNaso approved these changes Jul 18, 2018
View reviewed changes
Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

LGTM. @devsnek Sorry for late in review.

@devsnek
Copy link
Member Author

devsnek commented Jul 23, 2018

anything else that needs to happen here?

@mhdawson
Copy link
Member

I'd like to find time to take a closer unless someone beats me to landing it.

@devsnek
Copy link
Member Author

devsnek commented Aug 9, 2018 •
edited
Loading

ping @mhdawson

@mhdawson
Copy link
Member

Sorry still recovering from being out last week, once I catch up will try to get back to this.

@devsnek
Copy link
Member Author

devsnek commented Sep 6, 2018

ping @nodejs/addon-api

@Trott
Copy link
Member

Trott commented Sep 8, 2018

@nodejs/n-api Can this be merged now?

mhdawson
mhdawson approved these changes Sep 10, 2018
View reviewed changes
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 after requested doc update

@devsnek devsnek merged commit 2009c01 into nodejs:master Sep 18, 2018
@devsnek devsnek deleted the feature/bigint branch September 18, 2018 01:18
@mhdawson
Copy link
Member

This seems to have broken the CI see: https://ci.nodejs.org/job/node-test-node-addon-api/MACHINE=ubuntu1604-64/708/console

@mhdawson
Copy link
Member

@devsnek FYI

mhdawson added a commit to mhdawson/node-addon-api that referenced this pull request Sep 18, 2018
@mhdawson
src: fix compile failure in test
61ffacf 
Since original submit for
nodejs#292 warnings
were tightened through nodejs#315
causing the bigint test to fail to compile

Fix the compile failure
@mhdawson mhdawson mentioned this pull request Sep 18, 2018
Closed
@mhdawson
Copy link
Member

submitted #345 to fix compile failure.

mhdawson added a commit that referenced this pull request Sep 18, 2018
@mhdawson
src: fix compile failure in test
79ee838 
Since original submit for
#292 warnings
were tightened through #315
causing the bigint test to fail to compile

Fix the compile failure

PR-URL: #345
Reviewed-By: : None, landed to unbreak CI
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
src: fix compile failure in test
7f4abc4 
Since original submit for
nodejs/node-addon-api#292 warnings
were tightened through nodejs/node-addon-api#315
causing the bigint test to fail to compile

Fix the compile failure

PR-URL: nodejs/node-addon-api#345
Reviewed-By: : None, landed to unbreak CI
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
@Marlyfleitas
src: fix compile failure in test
d8058bd 
Since original submit for
nodejs/node-addon-api#292 warnings
were tightened through nodejs/node-addon-api#315
causing the bigint test to fail to compile

Fix the compile failure

PR-URL: nodejs/node-addon-api#345
Reviewed-By: : None, landed to unbreak CI
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
@wroy7860
src: fix compile failure in test
87f86da 
Since original submit for
nodejs/node-addon-api#292 warnings
were tightened through nodejs/node-addon-api#315
causing the bigint test to fail to compile

Fix the compile failure

PR-URL: nodejs/node-addon-api#345
Reviewed-By: : None, landed to unbreak CI
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
src: fix compile failure in test
156055b 
Since original submit for
nodejs/node-addon-api#292 warnings
were tightened through nodejs/node-addon-api#315
causing the bigint test to fail to compile

Fix the compile failure

PR-URL: nodejs/node-addon-api#345
Reviewed-By: : None, landed to unbreak CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@addaleax addaleax addaleax approved these changes

@NickNaso NickNaso NickNaso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@devsnek @mhdawson @Trott @NickNaso @addaleax