Skip to content

Compile on node 11.14 using nan v1.3.0 #223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ericwaldheim
Copy link
Contributor

Hello,
I failed to find any work done on a node 0.11.14 port so I modified the source to compile on node 0.11.14 by using nan v1.3.0.
I built and ran regression tests on both node 0.11.14 and 0.10.25.
(The database setup in test/outparams.js appears to be missing setup for procedure "procDateTimeOutParam".)

The modifications were straightforward except for:

  1. I replaced String::NewSymbol with NanNew. Nan docs indicate this is correct but I'd like to confirm.
  2. reader.cpp line 124 baton->callback.Clear(); I could not find the Nan replacement for the Clear call. I just blithely commented it out. What is the correct answer?
  3. statement.cpp line 94 baton->callback.Clear(); Same issue as number 2).

And I suppose question one is, how do you feel about nan?

Thank you,
Eric

@ericwaldheim
Copy link
Contributor Author

I looked into the Clear() issue further. Looks like the converted code with Clear removed is correct. Here's the translation in nan:

https://github.com/rvagg/nan/blob/d945fd11f83c6ba612c3eefcd579edaab5aa2e2e/nan.h#L1573-L1578

template<typename T>
  NAN_INLINE void NanDisposePersistent(
      v8::Persistent<T> &handle) {  // NOLINT(runtime/references)
    handle.Dispose();
    handle.Clear();
  }

@ericwaldheim
Copy link
Contributor Author

Here is the relevant comment from nan docs for "issue 1)" mentioned above. I believe this is converted correctly.
https://github.com/rvagg/nan#localstring-nansymbolconst-char-

@ericwaldheim ericwaldheim mentioned this pull request Apr 13, 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.

1 participant