-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
@@ -5,8 +5,6 @@ | |||
#pragma GCC diagnostic push | |||
#pragma GCC diagnostic ignored "-Wunused-parameter" |
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.
Check to see if nan.h
compiles cleanly without disabling these warnings.
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.
Nope.
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.
These were originally added because of unused params in V8 itself IIRC.
Huh, from a quick read-through usage of |
This is great btw, I had been meaning to do this but hadn't got around to it yet, I think this might even add |
Compile errors on iojs 2.5:
|
Node 0.12:
|
I think |
I wonder if the
|
@mikemorris AFAICT we shouldn't need to maintain a persistent handle to the request on the C++ side at all: we hand it off to the JS side and then it will be referenced by the callback's closure, so not GC'd:
I'm going to try switching |
I swear I remember testing and this being insufficient to prevent the request from being GC'd, worth double-checking though. EDIT: I think that was when we were still storing |
New error:
|
@jfirebaugh Perhaps |
I'm only seeing this on Node v0.10.x (on Linux and OS X), probably just missing a NAN wrapper somewhere. |
Fixed the |
Can we squash/merge this @jfirebaugh? My patch landed in |
^ squashed, merge if it's green. This is sweet! |
This is failing on render tests because the callback is getting called with an object that has "pixels": "pixels" -- yes, the value is literally the string "pixels" rather than a Buffer containing pixel data. I don't know why that's happening. Presumably something about how I'm using
Nan::NewBuffer
is incorrect but I don't see what.👀 @mikemorris