Make FromNode return std::optional

This change is required for converting types that do not have a default constructor, such as Dtype in MLX.
yue · Apr 12, 2024 · 273d2d6 · 273d2d6
1 parent 35c91ec
commit 273d2d6
Show file tree

Hide file tree

Showing 10 changed files with 217 additions and 171 deletions.
diff --git a/README.md b/README.md
@@ -25,11 +25,16 @@ __This project is at early stage, behavior of APIs may change without notice.__
   'include_dirs': ["<!(node -p \"require('kizunapi').include_dir\")"],
 ```
 
-3. Enable C++17 in `bindings.gyp` if you mean to support Node.js < 18:
+3. Enable C++17 in `bindings.gyp`:
 
 ```python
   'cflags_cc': [ '-std=c++17' ],
   'xcode_settings': { 'OTHER_CFLAGS': [ '-std=c++17'] },
+  'msvs_settings': {
+    'VCCLCompilerTool': {
+      'AdditionalOptions': [ '/std:c++17' ],
+    },
+  },
 ```
 
 4. In source code:

diff --git a/docs/tutorial.md b/docs/tutorial.md
@@ -113,9 +113,11 @@ struct Type<Point> {
       return napi_generic_failure;
     return napi_ok;
   }
-  static napi_status FromNode(napi_env env, napi_value value, Point* out) {
-    return Get(env, value, "x", &out->x, "y", &out->y) ? napi_ok
-                                                       : napi_generic_failure;
+  static std::optional<Point> FromNode(napi_env env, napi_value value) {
+    Point out;
+    if (Get(env, value, "x", &out.x, "y", &out.y))
+      return out;
+    return std::nullopt;
   }
 };
 

diff --git a/src/arguments.h b/src/arguments.h
@@ -68,28 +68,27 @@ class Arguments {
   }
 
   template<typename T>
-  bool GetNext(T* out) {
+  std::optional<T> GetNext() {
     if (next_ >= Length()) {
       insufficient_arguments_ = true;
-      return false;
+      return std::nullopt;
     }
-    return FromNode(env_, argv_[next_++], out);
+    return FromNode<T>(env_, argv_[next_++]);
   }
 
   // A helper to handle the cases where user wants to store a weak function
   // passed via arguments.
   template<typename Sig>
-  bool GetNextWeakFunction(std::function<Sig>* out) {
-    napi_value value;
-    if (!GetNext(&value))
-      return false;
-    return Type<std::function<Sig>>::FromNode(
-        env_, value, out, 0 /* ref_count */) == napi_ok;
+  std::optional<std::function<Sig>> GetNextWeakFunction() {
+    std::optional<napi_value> value = GetNext<napi_value>();
+    if (!value)
+      return std::nullopt;
+    return Type<std::function<Sig>>::FromNode(env_, *value, 0);
   }
 
   template<typename T>
-  bool GetThis(T* out) const {
-    return FromNode(env_, this_, out);
+  std::optional<T> GetThis() const {
+    return FromNode<T>(env_, this_);
   }
 
   bool IsConstructorCall() const {

diff --git a/src/callback.h b/src/callback.h
@@ -24,30 +24,27 @@ namespace ki {
 //   weakMap.get(win).set('onClick', callback)
 // In this way V8 can recognize the circular reference and do GC.
 template<typename ReturnType, typename... ArgTypes>
-inline napi_status ConvertWeakFunctionFromNode(
+std::optional<std::function<ReturnType(ArgTypes...)>>
+ConvertWeakFunctionFromNode(
     napi_env env,
     napi_value value,
-    std::function<ReturnType(ArgTypes...)>* out,
     int ref_count = 0 /* This parameter is only used internally */) {
   napi_valuetype type;
   napi_status s = napi_typeof(env, value, &type);
   if (s != napi_ok)
-    return s;
-  if (type == napi_null) {
-    *out = nullptr;
-    return napi_ok;
-  }
+    return std::nullopt;
   if (type != napi_function)
-    return napi_function_expected;
+    return std::nullopt;
+  if (type == napi_null)  // null is accepted as empty function
+    return nullptr;
   // Everything bundled with a std::function must be copiable, so we can not
   // simply move the handle here. And we would like to avoid actually copying
   // the Persistent because it requires a handle scope.
   auto handle = std::make_shared<Persistent>(env, value, ref_count);
-  *out = [env, handle](ArgTypes&&... args) -> ReturnType {
+  return [env, handle](ArgTypes&&... args) -> ReturnType {
     return internal::V8FunctionInvoker<ReturnType(ArgTypes...)>::Go(
         env, handle.get(), std::forward<ArgTypes>(args)...);
   };
-  return napi_ok;
 }
 
 // Define how callbacks are converted.
@@ -62,11 +59,10 @@ struct Type<std::function<ReturnType(ArgTypes...)>> {
       return napi_get_null(env, result);
     return internal::CreateNodeFunction(env, std::move(value), result);
   }
-  static napi_status FromNode(napi_env env,
-                              napi_value value,
-                              std::function<Sig>* out,
-                              int ref_count = 1 /* internal */) {
-    return ConvertWeakFunctionFromNode(env, value, out, ref_count);
+  static inline std::optional<std::function<Sig>> FromNode(
+      napi_env env, napi_value value, int ref_count = 1 /* internal */) {
+    return ConvertWeakFunctionFromNode<ReturnType, ArgTypes...>(
+        env, value, ref_count);
   }
 };
 

diff --git a/src/callback_internal.h b/src/callback_internal.h
@@ -25,7 +25,7 @@ namespace internal {
 // Deduce the proper type for callback parameters.
 template<typename T>
 struct CallbackParamTraits {
-  using LocalType = typename std::decay<T>::type;
+  using LocalType = std::decay_t<T>;
 };
 template<typename T>
 struct CallbackParamTraits<const T*> {
@@ -51,58 +51,80 @@ struct IsFunctionConversionSupported
 
 // Helper to read C++ args from JS args.
 template<typename T>
-inline bool GetNextArgument(Arguments* args, int flags, bool is_first,
-                            T* result) {
-  if (is_first && (flags & HolderIsFirstArgument) != 0) {
-    return args->GetThis(result);
-  } else {
-    return args->GetNext(result);
+struct ArgConverter {
+  static inline std::optional<T> GetNext(
+      Arguments* args, int flags, bool is_first) {
+    if (is_first && (flags & HolderIsFirstArgument) != 0) {
+      return args->GetThis<T>();
+    } else {
+      return args->GetNext<T>();
+    }
   }
-}
+};
+
+// Allow optional arguments.
+template<typename T>
+struct ArgConverter<std::optional<T>> {
+  static inline std::optional<std::optional<T>> GetNext(
+      Arguments* args, int flags, bool is_first) {
+    std::optional<T> out = ArgConverter<T>::GetNext(args, flags, is_first);
+    if (out)
+      return out;
+    else
+      return std::nullopt;
+  }
+};
 
 // Implementation of the FunctionArgumentIsWeakRef flag.
 template<typename Sig>
-inline bool GetNextArgument(Arguments* args, int flags, bool is_first,
-                            std::function<Sig>* result) {
-  if ((flags & FunctionArgumentIsWeakRef) != 0)
-    return args->GetNextWeakFunction(result);
-  else
-    return args->GetNext(result);
-}
+struct ArgConverter<std::function<Sig>> {
+  static inline std::optional<std::function<Sig>> GetNext(
+      Arguments* args, int flags, bool is_first) {
+    if ((flags & FunctionArgumentIsWeakRef) != 0)
+      return args->GetNextWeakFunction<Sig>();
+    else
+      return args->GetNext<std::function<Sig>>();
+  }
+};
 
 // For advanced use cases, we allow callers to request the unparsed Arguments
 // object and poke around in it directly.
-inline bool GetNextArgument(Arguments* args, int flags, bool is_first,
-                            Arguments* result) {
-  *result = *args;
-  return true;
-}
-inline bool GetNextArgument(Arguments* args, int flags, bool is_first,
-                            Arguments** result) {
-  *result = args;
-  return true;
-}
+template<>
+struct ArgConverter<Arguments> {
+  static inline std::optional<Arguments> GetNext(
+      Arguments* args, int flags, bool is_first) {
+    return *args;
+  }
+};
+template<>
+struct ArgConverter<Arguments*> {
+  static inline std::optional<Arguments*> GetNext(
+      Arguments* args, int flags, bool is_first) {
+    return args;
+  }
+};
 
 // It's common for clients to just need the env, so we make that easy.
-inline bool GetNextArgument(Arguments* args, int flags, bool is_first,
-                            napi_env* result) {
-  *result = args->Env();
-  return true;
-}
+template<>
+struct ArgConverter<napi_env> {
+  static inline std::optional<napi_env> GetNext(
+      Arguments* args, int flags, bool is_first) {
+    return args->Env();
+  }
+};
 
 // Class template for extracting and storing single argument for callback
 // at position |index|.
 template<size_t index, typename ArgType>
 struct ArgumentHolder {
-  using ArgLocalType = typename CallbackParamTraits<ArgType>::LocalType;
+  using LocalType = typename CallbackParamTraits<ArgType>::LocalType;
 
-  ArgLocalType value;
-  bool ok;
+  std::optional<LocalType> value;
 
   ArgumentHolder(Arguments* args, int flags)
-      : ok(GetNextArgument(args, flags, index == 0, &value)) {
-    if (!ok)
-      args->ThrowError(Type<ArgLocalType>::name);
+      : value(ArgConverter<LocalType>::GetNext(args, flags, index == 0)) {
+    if (!value)
+      args->ThrowError(Type<LocalType>::name);
   }
 };
 
@@ -169,13 +191,13 @@ class Invoker<IndicesHolder<indices...>, ArgTypes...>
       : ArgumentHolder<indices, ArgTypes>(args, flags)..., args_(args) {}
 
   bool IsOK() {
-    return And(ArgumentHolder<indices, ArgTypes>::ok...);
+    return And(ArgumentHolder<indices, ArgTypes>::value.has_value()...);
   }
 
   template<typename ReturnType>
   ReturnType DispatchToCallback(
       const std::function<ReturnType(ArgTypes...)>& callback) {
-    return callback(std::move(ArgumentHolder<indices, ArgTypes>::value)...);
+    return callback(std::move(*ArgumentHolder<indices, ArgTypes>::value)...);
   }
 
  private:

diff --git a/src/map.h b/src/map.h
@@ -40,8 +40,9 @@ class Map : public Local {
 
   template<typename K>
   bool Has(const K& key) const {
-    return FromNodeTo<bool>(
-        Env(), CallMethod(Env(), Value(), "has", ToNode(Env(), key)));
+    return FromNode<bool>(
+        Env(),
+        CallMethod(Env(), Value(), "has", ToNode(Env(), key))).value_or(false);
   }
 
   template<typename K>

diff --git a/src/prototype.h b/src/prototype.h
@@ -75,18 +75,16 @@ struct Type<T*, std::enable_if_t<!std::is_const_v<T> &&
     *result = object;
     return napi_ok;
   }
-  static inline napi_status FromNode(napi_env env, napi_value value, T** out) {
+  static inline std::optional<T*> FromNode(napi_env env, napi_value value) {
     void* result;
-    napi_status s = napi_unwrap(env, value, &result);
-    if (s != napi_ok)
-      return s;
+    if (napi_unwrap(env, value, &result) != napi_ok)
+      return std::nullopt;
     if (!internal::IsInstanceOf<T>(env, value))
-      return napi_generic_failure;
+      return std::nullopt;
     T* ptr = internal::Unwrap<T>::Do(result);
     if (!ptr)
-      return napi_generic_failure;
-    *out = ptr;
-    return napi_ok;
+      return std::nullopt;
+    return ptr;
   }
 };