src: minor cleanups on OneByteString usage

* Provide a OneByteString variant that accepts std::string_view * Use FIXED_ONE_BYTE_STRING in appropriate places Signed-off-by: James M Snell <[email protected]> PR-URL: #56482 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
nodejs · Jan 8, 2025 · 7b472fd · 7b472fd
1 parent c0ad193
commit 7b472fd
Show file tree

Hide file tree

Showing 18 changed files with 92 additions and 74 deletions.
diff --git a/src/crypto/crypto_ec.cc b/src/crypto/crypto_ec.cc
@@ -767,16 +767,16 @@ Maybe<void> ExportJWKEcKey(Environment* env,
   const int nid = EC_GROUP_get_curve_name(group);
   switch (nid) {
     case NID_X9_62_prime256v1:
-      crv_name = OneByteString(env->isolate(), "P-256");
+      crv_name = FIXED_ONE_BYTE_STRING(env->isolate(), "P-256");
       break;
     case NID_secp256k1:
-      crv_name = OneByteString(env->isolate(), "secp256k1");
+      crv_name = FIXED_ONE_BYTE_STRING(env->isolate(), "secp256k1");
       break;
     case NID_secp384r1:
-      crv_name = OneByteString(env->isolate(), "P-384");
+      crv_name = FIXED_ONE_BYTE_STRING(env->isolate(), "P-384");
       break;
     case NID_secp521r1:
-      crv_name = OneByteString(env->isolate(), "P-521");
+      crv_name = FIXED_ONE_BYTE_STRING(env->isolate(), "P-521");
       break;
     default: {
       THROW_ERR_CRYPTO_JWK_UNSUPPORTED_CURVE(

diff --git a/src/crypto/crypto_hash.cc b/src/crypto/crypto_hash.cc
@@ -152,7 +152,7 @@ void Hash::GetCachedAliases(const FunctionCallbackInfo<Value>& args) {
   names.reserve(size);
   values.reserve(size);
   for (auto& [alias, id] : env->alias_to_md_id_map) {
-    names.push_back(OneByteString(isolate, alias.c_str(), alias.size()));
+    names.push_back(OneByteString(isolate, alias));
     values.push_back(v8::Uint32::New(isolate, id));
   }
 #else

diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc
@@ -859,8 +859,7 @@ void TLSWrap::ClearOut() {
           if (context.IsEmpty()) [[unlikely]]
             return;
           const std::string error_str = GetBIOError();
-          Local<String> message = OneByteString(
-              env()->isolate(), error_str.c_str(), error_str.size());
+          Local<String> message = OneByteString(env()->isolate(), error_str);
           if (message.IsEmpty()) [[unlikely]]
             return;
           error = Exception::Error(message);
@@ -1974,7 +1973,7 @@ void TLSWrap::GetSharedSigalgs(const FunctionCallbackInfo<Value>& args) {
     } else {
       sig_with_md += "UNDEF";
     }
-    ret_arr[i] = OneByteString(env->isolate(), sig_with_md.c_str());
+    ret_arr[i] = OneByteString(env->isolate(), sig_with_md);
   }
 
   args.GetReturnValue().Set(

diff --git a/src/histogram.cc b/src/histogram.cc
@@ -341,7 +341,7 @@ Local<FunctionTemplate> IntervalHistogram::GetConstructorTemplate(
     Isolate* isolate = env->isolate();
     tmpl = NewFunctionTemplate(isolate, nullptr);
     tmpl->Inherit(HandleWrap::GetConstructorTemplate(env));
-    tmpl->SetClassName(OneByteString(isolate, "Histogram"));
+    tmpl->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "Histogram"));
     auto instance = tmpl->InstanceTemplate();
     instance->SetInternalFieldCount(HistogramImpl::kInternalFieldCount);
     HistogramImpl::AddMethods(isolate, tmpl);

diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc
@@ -333,7 +333,7 @@ void Url(const FunctionCallbackInfo<Value>& args) {
   if (url.empty()) {
     return;
   }
-  args.GetReturnValue().Set(OneByteString(env->isolate(), url.c_str()));
+  args.GetReturnValue().Set(OneByteString(env->isolate(), url));
 }
 
 void Initialize(Local<Object> target, Local<Value> unused,

diff --git a/src/node_builtins.cc b/src/node_builtins.cc
@@ -78,7 +78,7 @@ void BuiltinLoader::GetNatives(Local<Name> property,
   Local<Object> out = Object::New(isolate);
   auto source = env->builtin_loader()->source_.read();
   for (auto const& x : *source) {
-    Local<String> key = OneByteString(isolate, x.first.c_str(), x.first.size());
+    Local<String> key = OneByteString(isolate, x.first);
     out->Set(context, key, x.second.ToStringChecked(isolate)).FromJust();
   }
   info.GetReturnValue().Set(out);
@@ -207,7 +207,7 @@ MaybeLocal<String> BuiltinLoader::LoadBuiltinSource(Isolate* isolate,
                                     uv_err_name(r),
                                     uv_strerror(r),
                                     filename);
-    Local<String> message = OneByteString(isolate, buf.c_str());
+    Local<String> message = OneByteString(isolate, buf);
     isolate->ThrowException(v8::Exception::Error(message));
     return MaybeLocal<String>();
   }
@@ -277,8 +277,7 @@ MaybeLocal<Function> BuiltinLoader::LookupAndCompileInternal(
   }
 
   std::string filename_s = std::string("node:") + id;
-  Local<String> filename =
-      OneByteString(isolate, filename_s.c_str(), filename_s.size());
+  Local<String> filename = OneByteString(isolate, filename_s);
   ScriptOrigin origin(filename, 0, 0, true);
 
   BuiltinCodeCacheData cached_data{};
@@ -595,7 +594,7 @@ void BuiltinLoader::GetBuiltinCategories(
     return;
   if (result
           ->Set(context,
-                OneByteString(isolate, "cannotBeRequired"),
+                FIXED_ONE_BYTE_STRING(isolate, "cannotBeRequired"),
                 cannot_be_required_js)
           .IsNothing())
     return;
@@ -604,7 +603,7 @@ void BuiltinLoader::GetBuiltinCategories(
     return;
   if (result
           ->Set(context,
-                OneByteString(isolate, "canBeRequired"),
+                FIXED_ONE_BYTE_STRING(isolate, "canBeRequired"),
                 can_be_required_js)
           .IsNothing()) {
     return;
@@ -627,7 +626,7 @@ void BuiltinLoader::GetCacheUsage(const FunctionCallbackInfo<Value>& args) {
   }
   if (result
           ->Set(context,
-                OneByteString(isolate, "compiledWithCache"),
+                FIXED_ONE_BYTE_STRING(isolate, "compiledWithCache"),
                 builtins_with_cache_js)
           .IsNothing()) {
     return;
@@ -639,7 +638,7 @@ void BuiltinLoader::GetCacheUsage(const FunctionCallbackInfo<Value>& args) {
   }
   if (result
           ->Set(context,
-                OneByteString(isolate, "compiledWithoutCache"),
+                FIXED_ONE_BYTE_STRING(isolate, "compiledWithoutCache"),
                 builtins_without_cache_js)
           .IsNothing()) {
     return;
@@ -651,7 +650,7 @@ void BuiltinLoader::GetCacheUsage(const FunctionCallbackInfo<Value>& args) {
   }
   if (result
           ->Set(context,
-                OneByteString(isolate, "compiledInSnapshot"),
+                FIXED_ONE_BYTE_STRING(isolate, "compiledInSnapshot"),
                 builtins_in_snapshot_js)
           .IsNothing()) {
     return;

diff --git a/src/node_constants.cc b/src/node_constants.cc
@@ -1337,33 +1337,47 @@ void CreatePerContextProperties(Local<Object> target,
   // Define libuv constants.
   NODE_DEFINE_CONSTANT(os_constants, UV_UDP_REUSEADDR);
 
-  os_constants->Set(env->context(),
-                    OneByteString(isolate, "dlopen"),
-                    dlopen_constants).Check();
-  os_constants->Set(env->context(),
-                    OneByteString(isolate, "errno"),
-                    err_constants).Check();
-  os_constants->Set(env->context(),
-                    OneByteString(isolate, "signals"),
-                    sig_constants).Check();
-  os_constants->Set(env->context(),
-                    OneByteString(isolate, "priority"),
-                    priority_constants).Check();
-  target->Set(env->context(),
-              OneByteString(isolate, "os"),
-              os_constants).Check();
-  target->Set(env->context(),
-              OneByteString(isolate, "fs"),
-              fs_constants).Check();
-  target->Set(env->context(),
-              OneByteString(isolate, "crypto"),
-              crypto_constants).Check();
-  target->Set(env->context(),
-              OneByteString(isolate, "zlib"),
-              zlib_constants).Check();
-  target->Set(env->context(),
-              OneByteString(isolate, "trace"),
-              trace_constants).Check();
+  os_constants
+      ->Set(env->context(),
+            FIXED_ONE_BYTE_STRING(isolate, "dlopen"),
+            dlopen_constants)
+      .Check();
+  os_constants
+      ->Set(env->context(),
+            FIXED_ONE_BYTE_STRING(isolate, "errno"),
+            err_constants)
+      .Check();
+  os_constants
+      ->Set(env->context(),
+            FIXED_ONE_BYTE_STRING(isolate, "signals"),
+            sig_constants)
+      .Check();
+  os_constants
+      ->Set(env->context(),
+            FIXED_ONE_BYTE_STRING(isolate, "priority"),
+            priority_constants)
+      .Check();
+  target
+      ->Set(env->context(), FIXED_ONE_BYTE_STRING(isolate, "os"), os_constants)
+      .Check();
+  target
+      ->Set(env->context(), FIXED_ONE_BYTE_STRING(isolate, "fs"), fs_constants)
+      .Check();
+  target
+      ->Set(env->context(),
+            FIXED_ONE_BYTE_STRING(isolate, "crypto"),
+            crypto_constants)
+      .Check();
+  target
+      ->Set(env->context(),
+            FIXED_ONE_BYTE_STRING(isolate, "zlib"),
+            zlib_constants)
+      .Check();
+  target
+      ->Set(env->context(),
+            FIXED_ONE_BYTE_STRING(isolate, "trace"),
+            trace_constants)
+      .Check();
 }
 
 }  // namespace constants

diff --git a/src/node_errors.h b/src/node_errors.h
@@ -118,7 +118,7 @@ void OOMErrorHandler(const char* location, const v8::OOMDetails& details);
   inline v8::Local<v8::Object> code(                                           \
       v8::Isolate* isolate, const char* format, Args&&... args) {              \
     std::string message = SPrintF(format, std::forward<Args>(args)...);        \
-    v8::Local<v8::String> js_code = OneByteString(isolate, #code);             \
+    v8::Local<v8::String> js_code = FIXED_ONE_BYTE_STRING(isolate, #code);     \
     v8::Local<v8::String> js_msg =                                             \
         v8::String::NewFromUtf8(isolate,                                       \
                                 message.c_str(),                               \
@@ -129,7 +129,7 @@ void OOMErrorHandler(const char* location, const v8::OOMDetails& details);
                                   ->ToObject(isolate->GetCurrentContext())     \
                                   .ToLocalChecked();                           \
     e->Set(isolate->GetCurrentContext(),                                       \
-           OneByteString(isolate, "code"),                                     \
+           FIXED_ONE_BYTE_STRING(isolate, "code"),                             \
            js_code)                                                            \
         .Check();                                                              \
     return e;                                                                  \

diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -725,13 +725,14 @@ MaybeLocal<Object> Http2SessionPerformanceEntryTraits::GetDetails(
   SET(stream_average_duration_string, stream_average_duration)
   SET(stream_count_string, stream_count)
 
-  if (!obj->Set(
-          env->context(),
-          env->type_string(),
-          OneByteString(
-              env->isolate(),
-              (entry.details.session_type == NGHTTP2_SESSION_SERVER)
-                  ? "server" : "client")).IsJust()) {
+  if (!obj->Set(env->context(),
+                env->type_string(),
+                FIXED_ONE_BYTE_STRING(
+                    env->isolate(),
+                    (entry.details.session_type == NGHTTP2_SESSION_SERVER)
+                        ? "server"
+                        : "client"))
+           .IsJust()) {
     return MaybeLocal<Object>();
   }
 

diff --git a/src/node_messaging.cc b/src/node_messaging.cc
@@ -1660,7 +1660,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data,
     Local<FunctionTemplate> t = FunctionTemplate::New(isolate);
     t->InstanceTemplate()->SetInternalFieldCount(
         JSTransferable::kInternalFieldCount);
-    t->SetClassName(OneByteString(isolate, "JSTransferable"));
+    t->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "JSTransferable"));
     isolate_data->set_js_transferable_constructor_template(t);
   }
 

diff --git a/src/node_perf.h b/src/node_perf.h
@@ -124,12 +124,12 @@ struct PerformanceEntry {
     }
 
     v8::Local<v8::Value> argv[] = {
-      OneByteString(env->isolate(), name.c_str()),
-      OneByteString(env->isolate(), GetPerformanceEntryTypeName(Traits::kType)),
-      v8::Number::New(env->isolate(), start_time),
-      v8::Number::New(env->isolate(), duration),
-      detail
-    };
+        OneByteString(env->isolate(), name),
+        OneByteString(env->isolate(),
+                      GetPerformanceEntryTypeName(Traits::kType)),
+        v8::Number::New(env->isolate(), start_time),
+        v8::Number::New(env->isolate(), duration),
+        detail};
 
     node::MakeSyncCallback(
         env->isolate(),

diff --git a/src/node_process_events.cc b/src/node_process_events.cc
@@ -21,8 +21,7 @@ using v8::Value;
 Maybe<bool> ProcessEmitWarningSync(Environment* env, std::string_view message) {
   Isolate* isolate = env->isolate();
   Local<Context> context = env->context();
-  Local<String> message_string =
-      OneByteString(isolate, message.data(), message.size());
+  Local<String> message_string = OneByteString(isolate, message);
 
   Local<Value> argv[] = {message_string};
   Local<Function> emit_function = env->process_emit_warning_sync();

diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc
@@ -312,12 +312,12 @@ static void GetActiveResourcesInfo(const FunctionCallbackInfo<Value>& args) {
   // Active timeouts
   resources_info.insert(resources_info.end(),
                         env->timeout_info()[0],
-                        OneByteString(env->isolate(), "Timeout"));
+                        FIXED_ONE_BYTE_STRING(env->isolate(), "Timeout"));
 
   // Active immediates
   resources_info.insert(resources_info.end(),
                         env->immediate_info()->ref_count(),
-                        OneByteString(env->isolate(), "Immediate"));
+                        FIXED_ONE_BYTE_STRING(env->isolate(), "Immediate"));
 
   args.GetReturnValue().Set(
       Array::New(env->isolate(), resources_info.data(), resources_info.size()));

diff --git a/src/node_process_object.cc b/src/node_process_object.cc
@@ -104,12 +104,10 @@ static void SetVersions(Isolate* isolate, Local<Object> versions) {
 
   for (const auto& version : versions_array) {
     versions
-        ->DefineOwnProperty(
-            context,
-            OneByteString(isolate, version.first.data(), version.first.size()),
-            OneByteString(
-                isolate, version.second.data(), version.second.size()),
-            v8::ReadOnly)
+        ->DefineOwnProperty(context,
+                            OneByteString(isolate, version.first),
+                            OneByteString(isolate, version.second),
+                            v8::ReadOnly)
         .Check();
   }
 }

diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc
@@ -1735,7 +1735,7 @@ static void Initialize(Local<Object> target,
                          "StatementSync",
                          StatementSync::GetConstructorTemplate(env));
 
-  target->Set(context, OneByteString(isolate, "constants"), constants).Check();
+  target->Set(context, env->constants_string(), constants).Check();
 }
 
 }  // namespace sqlite

diff --git a/src/util-inl.h b/src/util-inl.h
@@ -180,6 +180,11 @@ inline v8::Local<v8::String> OneByteString(v8::Isolate* isolate,
       .ToLocalChecked();
 }
 
+inline v8::Local<v8::String> OneByteString(v8::Isolate* isolate,
+                                           std::string_view str) {
+  return OneByteString(isolate, str.data(), str.size());
+}
+
 char ToLower(char c) {
   return std::tolower(c, std::locale::classic());
 }

diff --git a/src/util.h b/src/util.h
@@ -340,6 +340,9 @@ inline v8::Local<v8::String> OneByteString(v8::Isolate* isolate,
                                            const unsigned char* data,
                                            int length = -1);
 
+inline v8::Local<v8::String> OneByteString(v8::Isolate* isolate,
+                                           std::string_view str);
+
 // Used to be a macro, hence the uppercase name.
 template <int N>
 inline v8::Local<v8::String> FIXED_ONE_BYTE_STRING(

diff --git a/src/uv.cc b/src/uv.cc
@@ -130,7 +130,7 @@ void Initialize(Local<Object> target,
   for (size_t i = 0; i < errors_len; ++i) {
     const auto& error = per_process::uv_errors_map[i];
     const std::string prefixed_name = prefix + error.name;
-    Local<String> name = OneByteString(isolate, prefixed_name.c_str());
+    Local<String> name = OneByteString(isolate, prefixed_name);
     Local<Integer> value = Integer::New(isolate, error.value);
     target->DefineOwnProperty(context, name, value, attributes).Check();
   }