Updates for review feedback (nodejs#199)

- Delete the node_api_*internal.h headers - Don't crash when casting numbers when V8_ENABLE_CHECKS is defined - Return 0 when getting NaN value as an integer - Always set callback data internal field to prevent crashing when V8_ENABLE_CHECKS is defined - Style: replace `if (ptr)` with `if (ptr != nullptr)` - Fix extern "C" in node_api_async.h - Minor doc changes
boingoing · Mar 25, 2017 · d713ae3 · d713ae3
1 parent c1226c7
commit d713ae3
Show file tree

Hide file tree

Showing 8 changed files with 60 additions and 75 deletions.
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -146,10 +146,11 @@ Silence all process warnings (including deprecations).
 
 ### `--napi-modules`
 <!-- YAML
-added: v8.0.0
+added: REPLACEME
 -->
 
-Load N-API modules (experimental, opt-in by adding this flag).
+Enable loading native modules compiled with the ABI-stable Node.js API (N-API)
+(experimental).
 
 ### `--trace-warnings`
 <!-- YAML

diff --git a/node.gyp b/node.gyp
@@ -165,11 +165,9 @@
         'src/node.cc',
         'src/node_api.cc',
         'src/node_api.h',
-        'src/node_api_internal.h',
         'src/node_api_types.h',
         'src/node_api_async.cc',
         'src/node_api_async.h',
-        'src/node_api_async_internal.h',
         'src/node_api_async_types.h',
         'src/node_buffer.cc',
         'src/node_config.cc',

diff --git a/src/node_api.cc b/src/node_api.cc
@@ -13,8 +13,9 @@
 #include <node_object_wrap.h>
 #include <string.h>
 #include <algorithm>
+#include <cmath>
 #include <vector>
-#include "node_api_internal.h"
+#include "node_api.h"
 
 namespace v8impl {
 
@@ -422,13 +423,9 @@ v8::Local<v8::Object> CreateFunctionCallbackData(napi_env env,
   cbdata->SetInternalField(
       v8impl::kFunctionIndex,
       v8::External::New(isolate, reinterpret_cast<void*>(cb)));
-
-  if (data) {
-    cbdata->SetInternalField(
-        v8impl::kDataIndex,
-        v8::External::New(isolate, reinterpret_cast<void*>(data)));
-  }
-
+  cbdata->SetInternalField(
+      v8impl::kDataIndex,
+      v8::External::New(isolate, reinterpret_cast<void*>(data)));
   return cbdata;
 }
 
@@ -446,24 +443,21 @@ v8::Local<v8::Object> CreateAccessorCallbackData(napi_env env,
   otpl->SetInternalFieldCount(v8impl::kAccessorFieldCount);
   v8::Local<v8::Object> cbdata = otpl->NewInstance(context).ToLocalChecked();
 
-  if (getter) {
+  if (getter != nullptr) {
     cbdata->SetInternalField(
         v8impl::kGetterIndex,
         v8::External::New(isolate, reinterpret_cast<void*>(getter)));
   }
 
-  if (setter) {
+  if (setter != nullptr) {
     cbdata->SetInternalField(
         v8impl::kSetterIndex,
         v8::External::New(isolate, reinterpret_cast<void*>(setter)));
   }
 
-  if (data) {
-    cbdata->SetInternalField(
-        v8impl::kDataIndex,
-        v8::External::New(isolate, reinterpret_cast<void*>(data)));
-  }
-
+  cbdata->SetInternalField(
+      v8impl::kDataIndex,
+      v8::External::New(isolate, reinterpret_cast<void*>(data)));
   return cbdata;
 }
 
@@ -642,7 +636,7 @@ napi_status napi_create_function(napi_env env,
 
   return_value = scope.Escape(tpl->GetFunction());
 
-  if (utf8name) {
+  if (utf8name != nullptr) {
     v8::Local<v8::String> name_string;
     CHECK_NEW_FROM_UTF8(isolate, name_string, utf8name);
     return_value->SetName(name_string);
@@ -1541,6 +1535,8 @@ napi_status napi_get_value_double(napi_env env,
                                   double* result) {
   // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
   // JS exceptions.
+  CHECK_ARG(env);
+  CHECK_ARG(value);
   CHECK_ARG(result);
 
   v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(value);
@@ -1556,13 +1552,16 @@ napi_status napi_get_value_int32(napi_env env,
                                  int32_t* result) {
   // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
   // JS exceptions.
+  CHECK_ARG(env);
+  CHECK_ARG(value);
   CHECK_ARG(result);
 
   v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(value);
-
-  // Value.As<Int32> works when the Value is any kind of Number.
   RETURN_STATUS_IF_FALSE(val->IsNumber(), napi_number_expected);
-  *result = val.As<v8::Int32>()->Value();
+
+  v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(env);
+  v8::Local<v8::Context> context = isolate->GetCurrentContext();
+  *result = val->Int32Value(context).ToChecked();
 
   return napi_ok;
 }
@@ -1572,13 +1571,16 @@ napi_status napi_get_value_uint32(napi_env env,
                                   uint32_t* result) {
   // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
   // JS exceptions.
+  CHECK_ARG(env);
+  CHECK_ARG(value);
   CHECK_ARG(result);
 
   v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(value);
-
-  // Value.As<Uint32> works when the Value is any kind of Number.
   RETURN_STATUS_IF_FALSE(val->IsNumber(), napi_number_expected);
-  *result = val.As<v8::Uint32>()->Value();
+
+  v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(env);
+  v8::Local<v8::Context> context = isolate->GetCurrentContext();
+  *result = val->Uint32Value(context).ToChecked();
 
   return napi_ok;
 }
@@ -1588,20 +1590,31 @@ napi_status napi_get_value_int64(napi_env env,
                                  int64_t* result) {
   // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
   // JS exceptions.
+  CHECK_ARG(env);
+  CHECK_ARG(value);
   CHECK_ARG(result);
 
   v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(value);
-
-  // Value.As<Integer> works when the Value is any kind of Number.
   RETURN_STATUS_IF_FALSE(val->IsNumber(), napi_number_expected);
-  *result = val.As<v8::Integer>()->Value();
+
+  // v8::Value::IntegerValue() converts NaN to INT64_MIN, inconsistent with
+  // v8::Value::Int32Value() that converts NaN to 0. So special-case NaN here.
+  double doubleValue = val.As<v8::Number>()->Value();
+  if (std::isnan(doubleValue)) {
+    *result = 0;
+  } else {
+    v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(env);
+    v8::Local<v8::Context> context = isolate->GetCurrentContext();
+    *result = val->IntegerValue(context).ToChecked();
+  }
 
   return napi_ok;
 }
 
 napi_status napi_get_value_bool(napi_env env, napi_value value, bool* result) {
   // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
   // JS exceptions.
+  CHECK_ARG(value);
   CHECK_ARG(result);
 
   v8::Local<v8::Value> val = v8impl::V8LocalValueFromJsValue(value);
@@ -2229,7 +2242,7 @@ napi_status napi_create_buffer_copy(napi_env env,
   v8::Local<v8::Object> buffer = maybe.ToLocalChecked();
   *result = v8impl::JsValueFromV8LocalValue(buffer);
 
-  if (result_data) {
+  if (result_data != nullptr) {
     *result_data = node::Buffer::Data(buffer);
   }
 
@@ -2253,10 +2266,10 @@ napi_status napi_get_buffer_info(napi_env env,
   v8::Local<v8::Object> buffer =
       v8impl::V8LocalValueFromJsValue(value).As<v8::Object>();
 
-  if (data) {
+  if (data != nullptr) {
     *data = node::Buffer::Data(buffer);
   }
-  if (length) {
+  if (length != nullptr) {
     *length = node::Buffer::Length(buffer);
   }
 

diff --git a/src/node_api_async.cc b/src/node_api_async.cc
@@ -1,4 +1,13 @@
-#include "node_api_async_internal.h"
+#include "node_api_async.h"
+#include "uv.h"
+
+typedef struct napi_work_impl__ {
+  uv_work_t* work;
+  void* data;
+  void (*execute)(void* data);
+  void (*complete)(void* data);
+  void (*destroy)(void* data);
+} napi_work_impl;
 
 napi_work napi_create_async_work() {
   napi_work_impl* worker =

diff --git a/src/node_api_async.h b/src/node_api_async.h
@@ -5,14 +5,16 @@
 #include "node_api.h"
 #include "node_api_async_types.h"
 
-extern "C" {
+EXTERN_C_START
+
 NAPI_EXTERN napi_work napi_create_async_work();
 NAPI_EXTERN void napi_delete_async_work(napi_work w);
 NAPI_EXTERN void napi_async_set_data(napi_work w, void* data);
 NAPI_EXTERN void napi_async_set_execute(napi_work w, void (*execute)(void*));
 NAPI_EXTERN void napi_async_set_complete(napi_work w, void (*complete)(void*));
 NAPI_EXTERN void napi_async_set_destroy(napi_work w, void (*destroy)(void*));
 NAPI_EXTERN void napi_async_queue_worker(napi_work w);
-}  // extern "C"
+
+EXTERN_C_END
 
 #endif  // SRC_NODE_API_ASYNC_H_
diff --git a/src/node_api_async_internal.h b/src/node_api_async_internal.h
diff --git a/src/node_api_internal.h b/src/node_api_internal.h
diff --git a/test/addons-napi/test_conversions/test.js b/test/addons-napi/test_conversions/test.js
@@ -32,6 +32,7 @@ assert.throws(() => test.asBool(testSym), boolExpected);
   assert.strictEqual(1, asInt(1.9));
   assert.strictEqual(0, asInt(0.9));
   assert.strictEqual(999, asInt(999.9));
+  assert.strictEqual(0, asInt(Number.NaN));
   assert.throws(() => asInt(undefined), numberExpected);
   assert.throws(() => asInt(null), numberExpected);
   assert.throws(() => asInt(false), numberExpected);
@@ -42,9 +43,6 @@ assert.throws(() => test.asBool(testSym), boolExpected);
   assert.throws(() => asInt(testSym), numberExpected);
 });
 
-assert.strictEqual(-Math.pow(2, 31), test.asInt32(Number.NaN));
-assert.strictEqual(0, test.asUInt32(Number.NaN));
-assert.strictEqual(-Math.pow(2, 63), test.asInt64(Number.NaN));
 assert.strictEqual(-1, test.asInt32(-1));
 assert.strictEqual(-1, test.asInt64(-1));
 assert.strictEqual(Math.pow(2, 32) - 1, test.asUInt32(-1));