Skip to content

Nan2 - #72

Closed
sonnyp wants to merge 5 commits into
masterfrom
nan2
Closed

Nan2#72
sonnyp wants to merge 5 commits into
masterfrom
nan2

Conversation

@sonnyp

@sonnyp sonnyp commented Sep 21, 2015

Copy link
Copy Markdown
Contributor

WIP; could use some help

Comment thread node-stringprep.cc Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this correct?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct but you should use Nan::Set(target, Nan::New<String>("StringPrep").ToLocalChecked(), t->GetFunction()); now for max-compatibility

@sonnyp sonnyp mentioned this pull request Sep 21, 2015
@austinkelleher

Copy link
Copy Markdown

+1 Need this very much. Looking to moving to Node 4.1.0.

@philidem

Copy link
Copy Markdown

👍

@erulabs

erulabs commented Sep 24, 2015

Copy link
Copy Markdown

👍 as well! Thank you for your efforts!

@lloydwatkin

Copy link
Copy Markdown
Contributor

@rvagg is usually very helpful with nan stuff if we ask nicely... please? :)

@erulabs

erulabs commented Sep 28, 2015

Copy link
Copy Markdown

@sonnyp I've started another effort here: #74

@austinkelleher

Copy link
Copy Markdown

The Nan2 support that @erulabs has started is looking pretty good so far.

Comment thread node-stringprep.cc Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be removed from inside all NAN_*() methods

@lloydwatkin

Copy link
Copy Markdown
Contributor

You rule @rvagg, thanks!

Comment thread node-stringprep.cc Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

../node-stringprep.cc: In static member function ‘static void StringPrep::Initialize(v8::Handle<v8::Object>)’:

../node-stringprep.cc:24:50: error: ‘NanAssignPersistent’ was not declared in this scope

     NanAssignPersistent(stringprep_constructor, t);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stringprep_constructor needs to be declared as a Nan::Persistent now (instead of just Persistent, i.e. v8::Persistent) and then you use stringprep_constructor.Reset(t) here

@sonnyp sonnyp mentioned this pull request Sep 29, 2015
Comment thread node-stringprep.cc Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

../node-stringprep.cc: In member function ‘v8::Local<v8::Value> StringPrep::prepare(v8::String::Value&)’:

../node-stringprep.cc:115:5: error: expected initializer before ‘size_t’

     size_t destLen = str.length() + 1;

@sonnyp

sonnyp commented Sep 29, 2015

Copy link
Copy Markdown
Contributor Author

@rvagg thanks a lot for the help

I ported @erulabs work from #74 and your comments here.

I added the last build errors as comments. Additionally if you could double check the PR that would be great.

@erulabs

erulabs commented Sep 29, 2015

Copy link
Copy Markdown

Hey @sonnyp looks like a missing semicolon on line #114

The remaining issue seems to be how we return the value (lines 228 & 279):

Local result = Nan::New(dest, destLen).ToLocalChecked();
info.GetReturnValue().Set(result);

This passes tests, but appears to break the unicode/ascii conversion. I tried a handful of ideas without much results. +1 to big big thanks to @rvagg 💃

@sonnyp

sonnyp commented Sep 30, 2015

Copy link
Copy Markdown
Contributor Author

@erulabs

../node-stringprep.cc: In function ‘Nan::NAN_METHOD_RETURN_TYPE ToUnicode(Nan::NAN_METHOD_ARGS_TYPE)’:

../node-stringprep.cc:228:11: error: missing template arguments before ‘result’

     Local result = Nan::New(dest, destLen).ToLocalChecked();

           ^

../node-stringprep.cc:228:11: error: expected ‘;’ before ‘result’

../node-stringprep.cc:230:31: error: ‘result’ was not declared in this scope

     info.GetReturnValue().Set(result);

https://github.com/node-xmpp/node-stringprep/pull/72/files#diff-4e17e0441c265ec99c496bc09f563bc2R228

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

8 participants