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

N-API Initial Patterns #458

Merged
merged 11 commits into from
Jan 17, 2020
Merged

N-API Initial Patterns #458

merged 11 commits into from
Jan 17, 2020

Conversation

kjvalencik
Copy link
Member

No description provided.

This was referenced Oct 25, 2019
Closed
Closed
kjvalencik
kjvalencik commented Oct 26, 2019
View reviewed changes
kjvalencik
kjvalencik commented Oct 26, 2019
View reviewed changes
kjvalencik
kjvalencik commented Oct 26, 2019
View reviewed changes
kjvalencik
kjvalencik commented Oct 26, 2019
View reviewed changes
kjvalencik
kjvalencik commented Oct 26, 2019
View reviewed changes
src/lib.rs
$crate::macro_internal::initialize_module(
// FIXME: Can we do without these?
std::mem::transmute(env),
std::mem::transmute(m),
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not as confident in this one, but I believe that this is simply an explicit version of the implicit type conversion happening in this definition:

                    register_func: Option<extern "C" fn(
                        $crate::handle::Handle<$crate::types::JsObject>, *mut u8, *mut u8)>,

@kjvalencik kjvalencik changed the title N-API WIP N-API Initial Patterns Oct 29, 2019
@kjvalencik kjvalencik marked this pull request as ready for review October 29, 2019 17:32
@kjvalencik kjvalencik mentioned this pull request Dec 7, 2019
Closed
@amilajack amilajack mentioned this pull request Dec 8, 2019
Closed
2 tasks
dherman
dherman requested changes Dec 13, 2019
View reviewed changes
Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

Awesome work, this will really help the N-API transition.

This was referenced Dec 23, 2019
Open
Open
kjvalencik and others added 2 commits December 23, 2019 16:09
@kjvalencik
Review Comments
82beaeb
@goto-bus-stop
ensure enough space for n-api to write a null terminator
18f042b 
[napi_get_value_string_utf8](https://nodejs.org/api/n-api.html#n_api_napi_get_value_string_utf8) wants to write a NULL terminator at the end, and truncates the string if it doesn't fit in the provided buffer. So we need to have an extra byte beyond the length of the string, so it can write the full thing.
dherman
dherman approved these changes Jan 17, 2020
View reviewed changes
Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

Looks great! Totally fine deferring some of the feedback to future issues.

@kjvalencik
Merge pull request #1 from goto-bus-stop/str-len
a3e40e6 
ensure enough space for n-api to write a null terminator
@kjvalencik kjvalencik merged commit f26fac9 into neon-bindings:master Jan 17, 2020
@dherman dherman mentioned this pull request Mar 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goto-bus-stop goto-bus-stop goto-bus-stop left review comments

@dherman dherman dherman 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.
3 participants
@kjvalencik @dherman @goto-bus-stop