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

Allow Value as target for function calls #499

Closed
wants to merge 3 commits into from
Closed

Allow Value as target for function calls #499

wants to merge 3 commits into from

Conversation

kkoopa
Copy link
Collaborator

@kkoopa kkoopa commented Oct 21, 2015

Fixes #497

R: @bnoordhuis

Also, Node should be fixed to allow v8::Value in node::MakeCallback.

bnoordhuis
bnoordhuis reviewed Oct 21, 2015
View reviewed changes
nan.h
, v8::Local<v8::String> symbol
, int argc
, v8::Local<v8::Value>* argv) {
#if NODE_MODULE_VERSION < IOJS_3_0_MODULE_VERSION
v8::Local<v8::Object> target_ = target->IsUndefined() || target->IsNull() ?

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@bnoordhuis
Copy link
Member

The ternaries are making my eyeballs bleed but LGTM.

@kkoopa kkoopa mentioned this pull request Oct 22, 2015
Closed
@kkoopa
Copy link
Collaborator Author

kkoopa commented Oct 22, 2015

Fixed the ugliness. Is this still good to go regarding MakeCallback?

@bnoordhuis
Copy link
Member

LGTM with the caveat that the handles from the Global() and ToObject() calls will leak into the handle scope of the caller. If that is bad, I'd add HandleScopes / EscapableHandleScopes.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Oct 22, 2015

But, but... They should not be creating new handles... I'll add the scopes immediately.

On October 22, 2015 11:21:59 PM GMT+03:00, Ben Noordhuis notifications@github.com wrote:

LGTM with the caveat that the handles from the Global() and ToObject()
calls will leak into the handle scope of the caller. If that is bad,
I'd add HandleScopes / EscapableHandleScopes.

Reply to this email directly or view it on GitHub:
#499 (comment)

@kkoopa
Copy link
Collaborator Author

kkoopa commented Oct 22, 2015

That should do it.

@kkoopa kkoopa added this to the 2.2 milestone Oct 22, 2015
@bnoordhuis
Copy link
Member

LGTM

@kkoopa
Copy link
Collaborator Author

kkoopa commented Oct 22, 2015

Thanks for taking the time to look through this. I'll flatten on merge.

@kkoopa kkoopa mentioned this pull request Oct 25, 2015
Closed
trevnorris
trevnorris reviewed Oct 26, 2015
View reviewed changes
nan.h
return New(node::MakeCallback(
v8::Isolate::GetCurrent(), target, func, argc, argv));
if (target->IsUndefined() || target->IsNull()) {
target_obj = isolate->GetCurrentContext()->Global();

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@kkoopa kkoopa closed this Nov 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.2
Development

Successfully merging this pull request may close these issues.

Receiver parameter for CallAsFunction should be Local<Value>
3 participants
@kkoopa @bnoordhuis @trevnorris