src: merge ToJsSet into ToV8Value

This addresses a `TODO` comment, and makes use of the opportunity to also clean up our `MaybeLocal` handling in this area and start accepting `std::string_view` where we accept `std::string`. PR-URL: nodejs#41757 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
jyxinye · Feb 13, 2022 · 34be1af · 34be1af
1 parent 60b8e79
commit 34be1af
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 29 deletions.
diff --git a/src/node_native_module_env.cc b/src/node_native_module_env.cc
@@ -22,17 +22,6 @@ using v8::SideEffectType;
 using v8::String;
 using v8::Value;
 
-// TODO(joyeecheung): make these more general and put them into util.h
-Local<Set> ToJsSet(Local<Context> context, const std::set<std::string>& in) {
-  Isolate* isolate = context->GetIsolate();
-  Local<Set> out = Set::New(isolate);
-  for (auto const& x : in) {
-    out->Add(context, OneByteString(isolate, x.c_str(), x.size()))
-        .ToLocalChecked();
-  }
-  return out;
-}
-
 bool NativeModuleEnv::Add(const char* id, const UnionBytes& source) {
   return NativeModuleLoader::GetInstance()->Add(id, source);
 }
@@ -67,16 +56,26 @@ void NativeModuleEnv::GetModuleCategories(
     cannot_be_required.insert("trace_events");
   }
 
-  result
+  Local<Value> cannot_be_required_js;
+  Local<Value> can_be_required_js;
+
+  if (!ToV8Value(context, cannot_be_required).ToLocal(&cannot_be_required_js))
+    return;
+  if (result
       ->Set(context,
             OneByteString(isolate, "cannotBeRequired"),
-            ToJsSet(context, cannot_be_required))
-      .FromJust();
-  result
+            cannot_be_required_js)
+      .IsNothing())
+    return;
+  if (!ToV8Value(context, can_be_required).ToLocal(&can_be_required_js))
+    return;
+  if (result
       ->Set(context,
             OneByteString(isolate, "canBeRequired"),
-            ToJsSet(context, can_be_required))
-      .FromJust();
+            can_be_required_js)
+      .IsNothing()) {
+    return;
+  }
   info.GetReturnValue().Set(result);
 }
 
@@ -85,23 +84,45 @@ void NativeModuleEnv::GetCacheUsage(const FunctionCallbackInfo<Value>& args) {
   Isolate* isolate = env->isolate();
   Local<Context> context = env->context();
   Local<Object> result = Object::New(isolate);
-  result
+
+  Local<Value> native_modules_with_cache_js;
+  Local<Value> native_modules_without_cache_js;
+  Local<Value> native_modules_in_snapshot_js;
+  if (!ToV8Value(context, env->native_modules_with_cache)
+      .ToLocal(&native_modules_with_cache_js)) {
+    return;
+  }
+  if (result
       ->Set(env->context(),
             OneByteString(isolate, "compiledWithCache"),
-            ToJsSet(context, env->native_modules_with_cache))
-      .FromJust();
-  result
+            native_modules_with_cache_js)
+      .IsNothing()) {
+    return;
+  }
+
+  if (!ToV8Value(context, env->native_modules_without_cache)
+      .ToLocal(&native_modules_without_cache_js)) {
+    return;
+  }
+  if (result
       ->Set(env->context(),
             OneByteString(isolate, "compiledWithoutCache"),
-            ToJsSet(context, env->native_modules_without_cache))
-      .FromJust();
+            native_modules_without_cache_js)
+      .IsNothing()) {
+    return;
+  }
 
-  result
+  if (!ToV8Value(context, env->native_modules_in_snapshot)
+      .ToLocal(&native_modules_without_cache_js)) {
+    return;
+  }
+  if (result
       ->Set(env->context(),
             OneByteString(isolate, "compiledInSnapshot"),
-            ToV8Value(env->context(), env->native_modules_in_snapshot)
-                .ToLocalChecked())
-      .FromJust();
+            native_modules_without_cache_js)
+      .IsNothing()) {
+    return;
+  }
 
   args.GetReturnValue().Set(result);
 }

diff --git a/src/util-inl.h b/src/util-inl.h
@@ -404,7 +404,7 @@ inline char* UncheckedCalloc(size_t n) { return UncheckedCalloc<char>(n); }
 void ThrowErrStringTooLong(v8::Isolate* isolate);
 
 v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
-                                    const std::string& str,
+                                    std::string_view str,
                                     v8::Isolate* isolate) {
   if (isolate == nullptr) isolate = context->GetIsolate();
   if (UNLIKELY(str.size() >= static_cast<size_t>(v8::String::kMaxLength))) {
@@ -436,6 +436,25 @@ v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
   return handle_scope.Escape(v8::Array::New(isolate, arr.out(), arr.length()));
 }
 
+template <typename T>
+v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
+                                    const std::set<T>& set,
+                                    v8::Isolate* isolate) {
+  if (isolate == nullptr) isolate = context->GetIsolate();
+  v8::Local<v8::Set> set_js = v8::Set::New(isolate);
+  v8::HandleScope handle_scope(isolate);
+
+  for (const T& entry : set) {
+    v8::Local<v8::Value> value;
+    if (!ToV8Value(context, entry, isolate).ToLocal(&value))
+      return {};
+    if (set_js->Add(context, value).IsEmpty())
+      return {};
+  }
+
+  return set_js;
+}
+
 template <typename T, typename U>
 v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
                                     const std::unordered_map<T, U>& map,

diff --git a/src/util.h b/src/util.h
@@ -36,7 +36,9 @@
 #include <limits>
 #include <memory>
 #include <string>
+#include <string_view>
 #include <type_traits>
+#include <set>
 #include <unordered_map>
 #include <utility>
 #include <vector>
@@ -644,7 +646,7 @@ using DeleteFnPtr = typename FunctionDeleter<T, function>::Pointer;
 std::vector<std::string> SplitString(const std::string& in, char delim);
 
 inline v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
-                                           const std::string& str,
+                                           std::string_view str,
                                            v8::Isolate* isolate = nullptr);
 template <typename T, typename test_for_number =
     typename std::enable_if<std::numeric_limits<T>::is_specialized, bool>::type>
@@ -655,6 +657,10 @@ template <typename T>
 inline v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
                                            const std::vector<T>& vec,
                                            v8::Isolate* isolate = nullptr);
+template <typename T>
+inline v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
+                                           const std::set<T>& set,
+                                           v8::Isolate* isolate = nullptr);
 template <typename T, typename U>
 inline v8::MaybeLocal<v8::Value> ToV8Value(v8::Local<v8::Context> context,
                                            const std::unordered_map<T, U>& map,