-
Notifications
You must be signed in to change notification settings - Fork 123
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
First step on N-API porting #196
base: master
Are you sure you want to change the base?
Conversation
@bnoordhuis are you going to have a chance to take a look at this updated version? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments but mostly LGTM. Thanks for doing this!
rc->Set(0, Nan::New<Integer>(static_cast<uint32_t>(input_consumed))); | ||
rc->Set(1, Nan::New<Integer>(static_cast<uint32_t>(output_consumed))); | ||
info.GetReturnValue().Set(errorno); | ||
rc.Set(static_cast<uint32_t>(0), Napi::Number::New(env, static_cast<uint32_t>(input_consumed))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my edification, this static_cast<uint32_t>(0)
is necessary because rc.Set(0, ...)
is ambiguous w.r.t. to the overload that takes a char pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's necessary because you can only pass this types to Set
method:
napi_value
Napi::Value
const char*
const std::string&
uint32_t
in alternative it could be possible do the same thing like reported below:
rc.Set(Napi::Number::New(env, 0), Napi::Number::New(env, static_cast<uint32_t>(input_consumed)));
rc.Set(Napi::Number::New(env, 1), Napi::Number::New(env, static_cast<uint32_t>(output_consumed)));
What do you prefer?
@bnoordhuis any chance you can review again after @NickNaso rebases. |
@NickNaso Sorry for the delay. Can you make sure you rebase against rather than merge in master? Your PR currently has some changes intermixed that already exist in master. I tried to rebase it myself but there are conflicts in src/binding.cc. |
@bnoordhuis Sorry for delay on working on this. Could you take a look and send your feedback? |
Hi everyone,
I'm opening a new PR about the N-API porting because I want work on this work and I don't have write access to the previous PR (#189) - I tried to contact the developer that started the the porting work but I never received any answer.
If possible please review this PR to move forward on ending this porting.