Address some minor PR feedback

 - Simplify conversion to/from napi_value
 - Better names for CallbackWrapper template params
 - Fatal error when trying to set setter return value
 - Use node::arraysize
 - Replace v8::Handle with v8::Local
 - Align the star after v8::Isolate
 - Remove some non-ascii whitespace chars that were
   causing some IDEs to auto-add a UTF-8 BOM

Author: jasongin

src/node_api.cc

@@ -74,41 +74,19 @@ V8EscapableHandleScopeFromJsEscapableHandleScope(
 
 //=== Conversion between V8 Handles and napi_value ========================
 
-// This is assuming v8::Local<> will always be implemented with a single
-// pointer field so that we can pass it around as a void* (maybe we should
-// use intptr_t instead of void*)
+// This asserts v8::Local<> will always be implemented with a single
+// pointer field so that we can pass it around as a void*.
+static_assert(sizeof(v8::Local<v8::Value>) == sizeof(napi_value),
+  "Cannot convert between v8::Local<v8::Value> and napi_value");
 
 napi_value JsValueFromV8LocalValue(v8::Local<v8::Value> local) {
-  // This is awkward, better way? memcpy but don't want that dependency?
-  union U {
-    napi_value v;
-    v8::Local<v8::Value> l;
-    U(v8::Local<v8::Value> _l) : l(_l) {}
-  } u(local);
-  assert(sizeof(u.v) == sizeof(u.l));
-  return u.v;
+  return reinterpret_cast<napi_value>(*local);
 }
 
 v8::Local<v8::Value> V8LocalValueFromJsValue(napi_value v) {
-  // Likewise awkward
-  union U {
-    napi_value v;
-    v8::Local<v8::Value> l;
-    U(napi_value _v) : v(_v) {}
-  } u(v);
-  assert(sizeof(u.v) == sizeof(u.l));
-  return u.l;
-}
-
-static v8::Local<v8::Function> V8LocalFunctionFromJsValue(napi_value v) {
-  // Likewise awkward
-  union U {
-    napi_value v;
-    v8::Local<v8::Function> f;
-    U(napi_value _v) : v(_v) {}
-  } u(v);
-  assert(sizeof(u.v) == sizeof(u.f));
-  return u.f;
+  v8::Local<v8::Value> local;
+  memcpy(&local, &v, sizeof(v));
+  return local;
 }
 
 // Wrapper around v8::Persistent that implements reference counting.
@@ -254,10 +232,10 @@ class CallbackWrapper {
   void* _data;
 };
 
-template <typename T, int I>
+template <typename Info, int InternalFieldIndex>
 class CallbackWrapperBase : public CallbackWrapper {
  public:
-  CallbackWrapperBase(const T& cbinfo, const int argsLength)
+  CallbackWrapperBase(const Info& cbinfo, const int argsLength)
       : CallbackWrapper(JsValueFromV8LocalValue(cbinfo.This()),
                         argsLength,
                         nullptr),
@@ -280,7 +258,8 @@ class CallbackWrapperBase : public CallbackWrapper {
     napi_callback_info cbinfo_wrapper = reinterpret_cast<napi_callback_info>(
         static_cast<CallbackWrapper*>(this));
     napi_callback cb = reinterpret_cast<napi_callback>(
-        v8::Local<v8::External>::Cast(_cbdata->GetInternalField(I))->Value());
+        v8::Local<v8::External>::Cast(
+            _cbdata->GetInternalField(InternalFieldIndex))->Value());
     v8::Isolate* isolate = _cbinfo.GetIsolate();
     cb(v8impl::JsEnvFromV8Isolate(isolate), cbinfo_wrapper);
 
@@ -291,7 +270,7 @@ class CallbackWrapperBase : public CallbackWrapper {
     }
   }
 
-  const T& _cbinfo;
+  const Info& _cbinfo;
   const v8::Local<v8::Object> _cbdata;
 };
 
@@ -399,8 +378,8 @@ class SetterCallbackWrapper
 
   /*virtual*/
   void SetReturnValue(napi_value v) override {
-    // Cannot set the return value of a setter.
-    assert(false);
+    node::FatalError("napi_set_return_value",
+      "Cannot return a value from a setter callback.");
   }
 
  private:
@@ -576,7 +555,7 @@ void napi_module_register(napi_module* mod) {
                          : napi_set_last_error(napi_pending_exception))
 
 // Static last error returned from napi_get_last_error_info
-napi_extended_error_info static_last_error = {};
+napi_extended_error_info static_last_error = { nullptr, nullptr, 0, napi_ok };
 
 // Warning: Keep in-sync with napi_status enum
 const char* error_messages[] = {nullptr,
@@ -598,8 +577,7 @@ void napi_clear_last_error() {
 }
 
 const napi_extended_error_info* napi_get_last_error_info() {
-  static_assert(
-      sizeof(error_messages) / sizeof(*error_messages) == napi_status_last,
+  static_assert(node::arraysize(error_messages) == napi_status_last,
       "Count of error messages must match count of error values");
   assert(static_last_error.error_code < napi_status_last);
 
@@ -629,7 +607,7 @@ napi_status napi_create_function(napi_env e,
   NAPI_PREAMBLE(e);
   CHECK_ARG(result);
 
-  v8::Isolate *isolate = v8impl::V8IsolateFromJsEnv(e);
+  v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(e);
   v8::Local<v8::Function> retval;
 
   v8::EscapableHandleScope scope(isolate);
@@ -774,7 +752,7 @@ napi_status napi_get_propertynames(napi_env e,
   NAPI_PREAMBLE(e);
   CHECK_ARG(result);
 
-  v8::Isolate *isolate = v8impl::V8IsolateFromJsEnv(e);
+  v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(e);
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
   v8::Local<v8::Object> obj;
   CHECK_TO_OBJECT(context, obj, object);
@@ -794,7 +772,7 @@ napi_status napi_set_property(napi_env e,
                               napi_value value) {
   NAPI_PREAMBLE(e);
 
-  v8::Isolate *isolate = v8impl::V8IsolateFromJsEnv(e);
+  v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(e);
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
   v8::Local<v8::Object> obj;
 
@@ -860,7 +838,7 @@ napi_status napi_set_named_property(napi_env e,
                                     napi_value value) {
   NAPI_PREAMBLE(e);
 
-  v8::Isolate *isolate = v8impl::V8IsolateFromJsEnv(e);
+  v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(e);
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
   v8::Local<v8::Object> obj;
 
@@ -1433,17 +1411,20 @@ napi_status napi_call_function(napi_env e,
   NAPI_PREAMBLE(e);
   CHECK_ARG(result);
 
-  std::vector<v8::Handle<v8::Value>> args(argc);
+  std::vector<v8::Local<v8::Value>> args(argc);
   v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(e);
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
 
-  v8::Handle<v8::Value> v8recv = v8impl::V8LocalValueFromJsValue(recv);
+  v8::Local<v8::Value> v8recv = v8impl::V8LocalValueFromJsValue(recv);
 
   for (int i = 0; i < argc; i++) {
     args[i] = v8impl::V8LocalValueFromJsValue(argv[i]);
   }
 
-  v8::Local<v8::Function> v8func = v8impl::V8LocalFunctionFromJsValue(func);
+  v8::Local<v8::Value> v8value = v8impl::V8LocalValueFromJsValue(func);
+  RETURN_STATUS_IF_FALSE(v8value->IsFunction(), napi_invalid_arg);
+
+  v8::Local<v8::Function> v8func = v8value.As<v8::Function>();
   auto maybe = v8func->Call(context, v8recv, argc, args.data());
 
   if (tryCatch.HasCaught()) {
@@ -1898,8 +1879,8 @@ napi_status napi_delete_reference(napi_env e, napi_ref ref) {
 // After this call the
 // reference will be a strong reference because its refcount is >0, and the
 // referenced object is
-// effectively "pinned". Calling this when the refcount is 0 and the object
-// is unavailable
+// effectively "pinned". Calling this when the refcount is 0 and the object
+// is unavailable
 // results in an error.
 napi_status napi_reference_addref(napi_env e, napi_ref ref, int* result) {
   NAPI_PREAMBLE(e);
@@ -1919,7 +1900,7 @@ napi_status napi_reference_addref(napi_env e, napi_ref ref, int* result) {
 // the result is
 // 0 the reference is now weak and the object may be GC'd at any time if there
 // are no other
-// references. Calling this when the refcount is already 0 results in an error.
+// references. Calling this when the refcount is already 0 results in an error.
 napi_status napi_reference_release(napi_env e, napi_ref ref, int* result) {
   NAPI_PREAMBLE(e);
   CHECK_ARG(ref);
@@ -1939,7 +1920,7 @@ napi_status napi_reference_release(napi_env e, napi_ref ref, int* result) {
 
 // Attempts to get a referenced value. If the reference is weak, the value might
 // no longer be
-// available, in that case the call is still successful but the result is NULL.
+// available, in that case the call is still successful but the result is NULL.
 napi_status napi_get_reference_value(napi_env e,
                                      napi_ref ref,
                                      napi_value* result) {
@@ -2017,13 +1998,15 @@ napi_status napi_new_instance(napi_env e,
   v8::Isolate* isolate = v8impl::V8IsolateFromJsEnv(e);
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
 
-  std::vector<v8::Handle<v8::Value>> args(argc);
+  std::vector<v8::Local<v8::Value>> args(argc);
   for (int i = 0; i < argc; i++) {
     args[i] = v8impl::V8LocalValueFromJsValue(argv[i]);
   }
 
-  v8::Local<v8::Function> v8cons =
-    v8impl::V8LocalFunctionFromJsValue(constructor);
+  v8::Local<v8::Value> v8value = v8impl::V8LocalValueFromJsValue(constructor);
+  RETURN_STATUS_IF_FALSE(v8value->IsFunction(), napi_invalid_arg);
+
+  v8::Local<v8::Function> v8cons = v8value.As<v8::Function>();
 
   auto maybe = v8cons->NewInstance(context, argc, args.data());
   CHECK_MAYBE_EMPTY(maybe, napi_generic_failure);
@@ -2102,12 +2085,12 @@ napi_status napi_make_callback(napi_env e,
       v8impl::V8LocalValueFromJsValue(recv).As<v8::Object>();
   v8::Local<v8::Function> v8func =
       v8impl::V8LocalValueFromJsValue(func).As<v8::Function>();
-  std::vector<v8::Handle<v8::Value>> args(argc);
+  std::vector<v8::Local<v8::Value>> args(argc);
   for (int i = 0; i < argc; i++) {
     args[i] = v8impl::V8LocalValueFromJsValue(argv[i]);
   }
 
-  v8::Handle<v8::Value> retval =
+  v8::Local<v8::Value> retval =
       node::MakeCallback(isolate, v8recv, v8func, argc, args.data());
   *result = v8impl::JsValueFromV8LocalValue(retval);
 

src/node_api.h

@@ -378,14 +378,14 @@ NAPI_EXTERN napi_status napi_reference_addref(napi_env env,
 // Decrements the reference count, optionally returning the resulting count.
 // If the result is 0 the reference is now weak and the object may be GC'd
 // at any time if there are no other references. Calling this when the
-// refcount is already 0 results in an error.
+// refcount is already 0 results in an error.
 NAPI_EXTERN napi_status napi_reference_release(napi_env env,
                                                napi_ref ref,
                                                int* result);
 
 // Attempts to get a referenced value. If the reference is weak,
 // the value might no longer be available, in that case the call
-// is still successful but the result is NULL.
+// is still successful but the result is NULL.
 NAPI_EXTERN napi_status napi_get_reference_value(napi_env env,
                                                  napi_ref ref,
                                                  napi_value* result);