X Tutup
The Wayback Machine - https://web.archive.org/web/20201231110156/https://github.com/nodejs/node-addon-examples/pull/126
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

Prevent asserting with significant expressions #126

Merged
merged 2 commits into from Feb 17, 2020
Merged

Conversation

@legendecas
Copy link
Member

@legendecas legendecas commented Feb 13, 2020

Also renamed category node-api to napi

Additional to #79.

@legendecas legendecas force-pushed the legendecas:assert branch from bf21900 to 98b5058 Feb 14, 2020
}

// This function runs on the main thread after `ExecuteWork` exits.
static void WorkComplete(napi_env env, napi_status status, void* data) {
AddonData* addon_data = (AddonData*)data;
napi_status s;

This comment has been minimized.

@mhdawson

mhdawson Feb 14, 2020
Member

A nit but I'd prefer a more complete name, for example status versus the one letter name

Copy link
Member

@mhdawson mhdawson left a comment

LGTM with suggestion

@legendecas legendecas force-pushed the legendecas:assert branch from 98b5058 to a2dff9f Feb 15, 2020
twoi and others added 2 commits Jan 29, 2019
so it runs in non-debug builds

PR-URL: #79
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Also renamed category node-api to napi
@legendecas legendecas force-pushed the legendecas:assert branch from a2dff9f to 90a5af4 Feb 15, 2020
@legendecas
Copy link
Member Author

@legendecas legendecas commented Feb 17, 2020

@mhdawson updated :)

@mhdawson mhdawson merged commit d883f37 into nodejs:master Feb 17, 2020
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@legendecas legendecas deleted the legendecas:assert branch Feb 18, 2020
@legendecas
Copy link
Member Author

@legendecas legendecas commented Feb 23, 2020

@mhdawson this PR contains another commit from #79 that I think it's worth to maintain the original authorship. WDYT? Maybe a force-push is needed.

@mhdawson
Copy link
Member

@mhdawson mhdawson commented Feb 24, 2020

@legendecas most often we squash all of the comments which is what I did in this case. I see there is a PR from another commit. We probably should have landed that separately by fixing up the conflicts in the PR if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.
X Tutup