Skip to content

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.

nan.h Outdated

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@bnoordhuis
Copy link
Member

The ternaries are making my eyeballs bleed but LGTM.

@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.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Receiver parameter for CallAsFunction should be Local<Value>

3 participants