Conversation
|
it works, thanks |
|
@tungj can you explain the static Persistent s_ct; part of the change? |
|
@Woodya I have no idea why he set |
|
@ripcurld00d so i've read that section now, and i don't yet see that Persistent is correct v Local in this case. this is not true in this case, at least afaict. The handle is only used inside the Initialize (static) method of each class, which in turn is called from the extern'd init(). I do see that various stackoverflow and web tutorials use the " .. s_ct = new .. (t)" pattern, but i don't see how any of them are more correct. It seems to me that they simply burden the system with a minor additional heap allocation. B/c unless a second function call will use the object, it's pointless.
however (and if i'm still wrong here, please point me in the right direction), i think they're simply re-using a pattern isn't appropriate, as they never use the s_ct outside their 1 function.
Also, per above comment about target->Set(), the commit actually leaves that reference as in short, i think it's an unnecessary involvement of the heap & additional unused symbols in the class. |
|
OK, I haven't done any deep investigation but from what I have seen so far, the |
This makes the Gzip, Gunzip, Bzip and Bunzip classes extend ObjectWrap instead of EventEmitter since the latter has been removed from node v0.5 and the stable v0.6.0, as described in #8
I'm not certain I'm doing this correctly, but 'node test.js' does work in both node v0.4.11 and v0.6.0 (assuming the symlink to gzbz2 is updated to point to build/debug instead of build/Release which v0.6.0 uses)