Skip to content
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

Migration to N-API #189

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions binding.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,24 @@
'targets': [
{
'target_name': 'iconv',
'include_dirs': ['<!(node -e "require(\'nan\')")'],
'cflags!': [ '-fno-exceptions' ],
Copy link

@gabrielschulhof gabrielschulhof Nov 9, 2018

Choose a reason for hiding this comment

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

The original version of node-iconv made sure there were no CPP exceptions. Thus, we should maintain that choice. This means that cflags! needs to be changed to cflags.

'cflags_cc!': [ '-fno-exceptions' ],

Choose a reason for hiding this comment

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

... and cflags_cc! needs to be changed to cflags_cc.

'xcode_settings': {
'GCC_ENABLE_CPP_EXCEPTIONS': 'YES',

Choose a reason for hiding this comment

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

... and this needs to be changed to 'NO'.

'CLANG_CXX_LIBRARY': 'libc++',
Copy link
Owner

Choose a reason for hiding this comment

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

Can you remove this? It's incompatible with Node.js v6.x. node-iconv can just inherit it from node's common.gypi.

'MACOSX_DEPLOYMENT_TARGET': '10.7',
'GCC_ENABLE_CPP_RTTI': 'NO',
'WARNING_CFLAGS': ['-Wall', '-Wextra', '-Wno-unused-parameter'],
},
'msvs_settings': {
'VCCLCompilerTool': { 'ExceptionHandling': 1 },
},
Copy link
Owner

Choose a reason for hiding this comment

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

This block should be removed.

'include_dirs': [
'<!@(node -p "require(\'node-addon-api\').include")',
'support'
],
'dependencies': [
"<!(node -p \"require('node-addon-api').gyp\")"],
Copy link
Owner

Choose a reason for hiding this comment

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

Can you use single quotes for consistency?

'sources': ['src/binding.cc'],
'ccflags': [
'-Wall',
Expand All @@ -15,12 +32,6 @@
'-fno-exceptions',
'-fno-rtti',
],
# Have to repeat flags on mac because of gyp's xcode emulation "feature".
'xcode_settings': {
'GCC_ENABLE_CPP_EXCEPTIONS': 'NO',
'GCC_ENABLE_CPP_RTTI': 'NO',
'WARNING_CFLAGS': ['-Wall', '-Wextra', '-Wno-unused-parameter'],
},
'conditions': [
['node_iconv_use_system_libiconv==0', {
'defines': ['ICONV_CONST=const', 'ENABLE_EXTRA=1'],
Expand Down
18 changes: 18 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"license": "ISC",
"readmeFilename": "README.md",
"dependencies": {
"nan": "^2.11.1",
"node-addon-api": "1.6.0",

Choose a reason for hiding this comment

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

This should be ^1.6.0 so that, when node-addon-api@1.7.0 comes out this package will start using it.

"safer-buffer": "^2.1.2"
},
"scripts": {
Expand Down
113 changes: 50 additions & 63 deletions src/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
*/

#include "iconv.h"
#include "nan.h"
#include "napi.h"
#include "uv.h"

Choose a reason for hiding this comment

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

@MrKashlakov why do you need "uv.h"?

#include "node_buffer.h"
Copy link

@gabrielschulhof gabrielschulhof Nov 9, 2018

Choose a reason for hiding this comment

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

@MrKashlakov I believe you can remove this include, because node-addon-api provides access to node::Buffer via N-API.


#include <errno.h>
Expand All @@ -26,89 +27,72 @@
#define ICONV_CONST
#endif // ICONV_CONST

namespace
{

using v8::Array;
using v8::Boolean;
using v8::FunctionTemplate;
using v8::Handle;
using v8::Integer;
using v8::Local;
using v8::Null;
using v8::Object;
using v8::ObjectTemplate;
using v8::String;
using v8::Value;

using namespace Napi;

Choose a reason for hiding this comment

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

You don't actually need this line, because the code uses Napi::... explicitly everywhere – which is good.


struct Iconv
struct Iconv: Napi::ObjectWrap<Iconv>
{
static Nan::Persistent<ObjectTemplate> object_template;
iconv_t conv_;
static Napi::FunctionReference object_template; // iconv_constructor

Iconv(iconv_t conv)
{
conv_ = conv;
}
Iconv(const Napi::CallbackInfo& info):
Napi::ObjectWrap<Iconv>(info) {
conv_ = info[0].As<Napi::External<iconv_t>>().Data();
}
iconv_t conv_;
Copy link
Owner

Choose a reason for hiding this comment

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

Inconsistent indentation, and can you move conv_ back to its old place again? Cheers.


~Iconv()
{
iconv_close(conv_);
}

static void WeakCallback(const Nan::WeakCallbackInfo<Iconv>& data)
{
delete data.GetParameter();
static void Init(Napi::Env env) {
Napi::Function func = DefineClass(env, "Iconv", {});
Iconv::object_template = Napi::Persistent(func);
Iconv::object_template.SuppressDestruct();
}

static void Initialize(Handle<Object> obj)
static Napi::Object Initialize(Napi::Env env, Napi::Object exports)
{
Local<ObjectTemplate> t = Nan::New<ObjectTemplate>();
t->SetInternalFieldCount(1);
object_template.Reset(t);
obj->Set(Nan::New<String>("make").ToLocalChecked(),
Nan::New<FunctionTemplate>(Make)->GetFunction());
obj->Set(Nan::New<String>("convert").ToLocalChecked(),
Nan::New<FunctionTemplate>(Convert)->GetFunction());
Iconv::Init(env);
exports.Set(Napi::String::New(env, "make"),
Napi::Function::New(env, Make));

Choose a reason for hiding this comment

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

This line should be indented so the Napi::... is in the same column as the line above.

exports.Set(Napi::String::New(env, "convert"),
Napi::Function::New(env, Convert));

Choose a reason for hiding this comment

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

Same here wrt. indentation.

#define EXPORT_ERRNO(err) \
obj->Set(Nan::New<String>(#err).ToLocalChecked(), Nan::New<Integer>(err))
exports.Set(Napi::String::New(env, #err), Napi::Number::New(env, err))
EXPORT_ERRNO(EINVAL);
EXPORT_ERRNO(EILSEQ);
EXPORT_ERRNO(E2BIG);
#undef EXPORT_ERRNO
return exports;
}

static NAN_METHOD(Make)
static Napi::Value Make(const Napi::CallbackInfo& info)
{
Nan::Utf8String from_encoding(info[0]);
Nan::Utf8String to_encoding(info[1]);
iconv_t conv = iconv_open(*to_encoding, *from_encoding);
std::string from_encoding = info[0].As<Napi::String>();
std::string to_encoding = info[1].As<Napi::String>();
iconv_t conv = iconv_open(to_encoding.c_str(), from_encoding.c_str());
if (conv == reinterpret_cast<iconv_t>(-1)) {
return info.GetReturnValue().SetNull();
return info.Env().Null();
}
Iconv* iv = new Iconv(conv);
Local<Object> obj =
Nan::New<ObjectTemplate>(object_template)->NewInstance();
Nan::SetInternalFieldPointer(obj, 0, iv);
Nan::Persistent<Object> persistent(obj);
persistent.SetWeak(iv, WeakCallback, Nan::WeakCallbackType::kParameter);
info.GetReturnValue().Set(obj);
Napi::Value param = Napi::External<void>::New(info.Env(), conv);
return Iconv::object_template.New({param});
}

static NAN_METHOD(Convert)
static Napi::Value Convert(const Napi::CallbackInfo& info)
{
Iconv* iv = static_cast<Iconv*>(
Nan::GetInternalFieldPointer(info[0].As<Object>(), 0));
const bool is_flush = Nan::To<bool>(info[8]).FromJust();
Napi::Env env = info.Env();
Iconv* iv = Iconv::Unwrap(info[0].As<Napi::Object>());

const bool is_flush = info[8].As<Napi::Boolean>().Value();
ICONV_CONST char* input_buf =
is_flush ? NULL : node::Buffer::Data(info[1].As<Object>());
size_t input_start = Nan::To<uint32_t>(info[2]).FromJust();
size_t input_size = Nan::To<uint32_t>(info[3]).FromJust();
char* output_buf = node::Buffer::Data(info[4].As<Object>());
size_t output_start = Nan::To<uint32_t>(info[5]).FromJust();
size_t output_size = Nan::To<uint32_t>(info[6]).FromJust();
Local<Array> rc = info[7].As<Array>();
is_flush ? NULL : info[1].As<Napi::Buffer<ICONV_CONST char>>().Data();
size_t input_start = info[2].IsNumber() ? info[2].As<Napi::Number>().Uint32Value() : static_cast<uint32_t>(0);
size_t input_size = info[3].IsNumber() ? info[3].As<Napi::Number>().Uint32Value() : static_cast<uint32_t>(0);
char* output_buf = info[4].As<Napi::Buffer<char>>().Data();
size_t output_start = info[5].IsNumber() ? info[5].As<Napi::Number>().Uint32Value() : static_cast<uint32_t>(0);
size_t output_size = info[6].IsNumber() ? info[6].As<Napi::Number>().Uint32Value() : static_cast<uint32_t>(0);
Copy link
Owner

Choose a reason for hiding this comment

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

Why the ternaries? The JS layer always passes up numbers.

Napi::Array rc = info[7].As<Napi::Array>();
if (input_buf != NULL) input_buf += input_start;
output_buf += output_start;
size_t input_consumed = input_size;
Expand All @@ -124,18 +108,21 @@ struct Iconv
}
input_consumed -= input_size;
output_consumed -= output_size;
rc->Set(0, Nan::New<Integer>(static_cast<uint32_t>(input_consumed)));
rc->Set(1, Nan::New<Integer>(static_cast<uint32_t>(output_consumed)));
info.GetReturnValue().Set(errorno);
rc.Set(static_cast<uint32_t>(0), Napi::Number::New(env, static_cast<uint32_t>(input_consumed)));
rc.Set(static_cast<uint32_t>(1), Napi::Number::New(env, static_cast<uint32_t>(output_consumed)));
return Napi::Number::New(env, errorno);
}

// Forbid implicit copying.
Iconv(const Iconv&);
void operator=(const Iconv&);

Choose a reason for hiding this comment

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

This new empty line is not needed.

};

Nan::Persistent<ObjectTemplate> Iconv::object_template;
Napi::FunctionReference Iconv::object_template; // iconv_constructor

} // namespace
Napi::Object InitAll(Napi::Env env, Napi::Object exports) {
return Iconv::Initialize(env, exports);
}

NODE_MODULE(iconv, Iconv::Initialize);
NODE_API_MODULE(iconv, InitAll)