Skip to content

New weak API for new NAN version. - #80

Closed
kkoopa wants to merge 3 commits into
nodejs:0.8.1-wipfrom
kkoopa:possible_api
Closed

kkoopa wants to merge 3 commits into
nodejs:0.8.1-wipfrom
kkoopa:possible_api

Conversation

@kkoopa

@kkoopa kkoopa commented Mar 13, 2014

Copy link
Copy Markdown
Collaborator

Here is my proposal for the new weak API. This works in both 0.10 and newer 0.11. NanMakeWeak is now useless, so I removed it. NanInitPersistent never saw much appreciation, and since it can't really be used with weak handles any more, I decided to cut it too. Introduced NanMakeWeakPersistent, which makes a Persistent and sets it weak.

I wrote a test which actually does something useful and checks expected behavior of weak handles.

As several thing have been changed and removed, I suggest cleaning up the rest of NAN, making a clean break with outdated 0.11 versions. That will save us a lot of work. If we clean up, this is more or less done. If backwards compatibility (regarding NAN) should be maintained where possible, there is some fixing and testing left to do.

@bnoordhuis Can you review the changes already done? I might have done some horrible mistake, leaking memory or missed a cleaner way of achieving something.

@kkoopa kkoopa mentioned this pull request Mar 13, 2014
Comment thread nan.h

This comment was marked as off-topic.

This comment was marked as off-topic.

@bnoordhuis

Copy link
Copy Markdown
Member

The tests don't compile with g++ 4.8. 4.8 is more strict in what it accepts than previous versions of gcc. Here is the build error:

In file included from ../cpp/weak.cpp:9:0:
../../nan.h: In instantiation of 'v8::Local<T> _NanWeakCallbackData<T, P>::GetValue() const [with T = v8::Function; P = int]':
../cpp/weak.cpp:16:33:   required from 'void weakCallback(const _NanWeakCallbackData<T, P>&) [with T = v8::Function; P = int]'
../cpp/weak.cpp:11:1:   required from 'void _Nan_Weak_Callback_weakCallback(v8::Persistent<v8::Value>, void*) [with T = v8::Function; P = int]'
../cpp/weak.cpp:37:5:   required from here
../../nan.h:685:50: error: 'NanPersistentToLocal' was not declared in this scope, and no declarations were found by argument-dependent lookup at the point of instantiation [-fpermissive]
     return NanPersistentToLocal(info_->persistent);
                                                  ^
../../nan.h:879:41: note: 'template<class TypeName> v8::Local<T> NanPersistentToLocal(const v8::Persistent<S>&)' declared here, later in the translation unit
   static NAN_INLINE(v8::Local<TypeName> NanPersistentToLocal(
                                         ^
../../nan.h:148:40: note: in definition of macro 'NAN_INLINE'
 # define NAN_INLINE(declarator) inline declarator
                                        ^
make: *** [Debug/obj.target/weak/cpp/weak.o] Error 1

@kkoopa

kkoopa commented Mar 14, 2014

Copy link
Copy Markdown
Collaborator Author

I should have fixed everything in #84 now.

@kkoopa

kkoopa commented Mar 16, 2014

Copy link
Copy Markdown
Collaborator Author

Merged into #86

@kkoopa kkoopa closed this Mar 16, 2014
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.

2 participants