src,lib: group properties used as constants from util binding

Properties used as constants in `util` internal binding are scattered. This suggests using an object holding all of them for better maintenance. Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com> PR-URL: nodejs#45539 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
marco-ippolito · Nov 24, 2022 · 1e0aac3 · 1e0aac3
1 parent af60389
commit 1e0aac3
Show file tree

Hide file tree

Showing 7 changed files with 65 additions and 47 deletions.
diff --git a/lib/buffer.js b/lib/buffer.js
@@ -67,11 +67,11 @@ const {
   kStringMaxLength
 } = internalBinding('buffer');
 const {
-  getOwnNonIndexProperties,
-  propertyFilter: {
+  constants: {
     ALL_PROPERTIES,
-    ONLY_ENUMERABLE
+    ONLY_ENUMERABLE,
   },
+  getOwnNonIndexProperties,
 } = internalBinding('util');
 const {
   customInspectSymbol,

diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
@@ -80,12 +80,14 @@ const {
   validateInteger,
 } = require('internal/validators');
 const {
+  constants: {
+    kExitCode,
+    kExiting,
+    kHasExitCode,
+  },
   privateSymbols: {
     exit_info_private_symbol,
   },
-  kExitCode,
-  kExiting,
-  kHasExitCode,
 } = internalBinding('util');
 
 setupProcessObject();

diff --git a/lib/internal/util/comparisons.js b/lib/internal/util/comparisons.js
@@ -46,11 +46,11 @@ const {
   isFloat64Array,
 } = types;
 const {
-  getOwnNonIndexProperties,
-  propertyFilter: {
+  constants: {
     ONLY_ENUMERABLE,
-    SKIP_SYMBOLS
-  }
+    SKIP_SYMBOLS,
+  },
+  getOwnNonIndexProperties,
 } = internalBinding('util');
 
 const kStrict = true;

diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js
@@ -97,18 +97,18 @@ const {
 } = primordials;
 
 const {
+  constants: {
+    ALL_PROPERTIES,
+    ONLY_ENUMERABLE,
+    kPending,
+    kRejected,
+  },
   getOwnNonIndexProperties,
   getPromiseDetails,
   getProxyDetails,
-  kPending,
-  kRejected,
   previewEntries,
   getConstructorName: internalGetConstructorName,
   getExternalValue,
-  propertyFilter: {
-    ALL_PROPERTIES,
-    ONLY_ENUMERABLE
-  }
 } = internalBinding('util');
 
 const {

diff --git a/lib/internal/webstreams/util.js b/lib/internal/webstreams/util.js
@@ -40,8 +40,10 @@ const {
 } = require('util');
 
 const {
+  constants: {
+    kPending,
+  },
   getPromiseDetails,
-  kPending,
 } = internalBinding('util');
 
 const assert = require('internal/assert');

diff --git a/lib/repl.js b/lib/repl.js
@@ -166,11 +166,11 @@ const {
   setupReverseSearch,
 } = require('internal/repl/utils');
 const {
-  getOwnNonIndexProperties,
-  propertyFilter: {
+  constants: {
     ALL_PROPERTIES,
-    SKIP_SYMBOLS
-  }
+    SKIP_SYMBOLS,
+  },
+  getOwnNonIndexProperties,
 } = internalBinding('util');
 const {
   startSigintWatchdog,

diff --git a/src/node_util.cc b/src/node_util.cc
@@ -23,6 +23,7 @@ using v8::Isolate;
 using v8::KeyCollectionMode;
 using v8::Local;
 using v8::Object;
+using v8::ObjectTemplate;
 using v8::ONLY_CONFIGURABLE;
 using v8::ONLY_ENUMERABLE;
 using v8::ONLY_WRITABLE;
@@ -364,7 +365,7 @@ void Initialize(Local<Object> target,
   Isolate* isolate = env->isolate();
 
   {
-    Local<v8::ObjectTemplate> tmpl = v8::ObjectTemplate::New(isolate);
+    Local<ObjectTemplate> tmpl = ObjectTemplate::New(isolate);
 #define V(PropertyName, _)                                                     \
   tmpl->Set(FIXED_ONE_BYTE_STRING(env->isolate(), #PropertyName),              \
             env->PropertyName());
@@ -379,27 +380,50 @@ void Initialize(Local<Object> target,
         .Check();
   }
 
-#define V(name)                                                               \
-  target->Set(context,                                                        \
-              FIXED_ONE_BYTE_STRING(env->isolate(), #name),                   \
-              Integer::New(env->isolate(), Promise::PromiseState::name))      \
-    .FromJust()
-  V(kPending);
-  V(kFulfilled);
-  V(kRejected);
+  {
+    Local<Object> constants = Object::New(isolate);
+#define V(name)                                                                \
+  constants                                                                    \
+      ->Set(context,                                                           \
+            FIXED_ONE_BYTE_STRING(isolate, #name),                             \
+            Integer::New(isolate, Promise::PromiseState::name))                \
+      .Check();
+
+    V(kPending);
+    V(kFulfilled);
+    V(kRejected);
 #undef V
 
 #define V(name)                                                                \
-  target                                                                       \
+  constants                                                                    \
       ->Set(context,                                                           \
-            FIXED_ONE_BYTE_STRING(env->isolate(), #name),                      \
-            Integer::New(env->isolate(), Environment::ExitInfoField::name))    \
-      .FromJust()
-  V(kExiting);
-  V(kExitCode);
-  V(kHasExitCode);
+            FIXED_ONE_BYTE_STRING(isolate, #name),                             \
+            Integer::New(isolate, Environment::ExitInfoField::name))           \
+      .Check();
+
+    V(kExiting);
+    V(kExitCode);
+    V(kHasExitCode);
 #undef V
 
+#define V(name)                                                                \
+  constants                                                                    \
+      ->Set(context,                                                           \
+            FIXED_ONE_BYTE_STRING(isolate, #name),                             \
+            Integer::New(isolate, PropertyFilter::name))                       \
+      .Check();
+
+    V(ALL_PROPERTIES);
+    V(ONLY_WRITABLE);
+    V(ONLY_ENUMERABLE);
+    V(ONLY_CONFIGURABLE);
+    V(SKIP_STRINGS);
+    V(SKIP_SYMBOLS);
+#undef V
+
+    target->Set(context, env->constants_string(), constants).Check();
+  }
+
   SetMethodNoSideEffect(
       context, target, "getPromiseDetails", GetPromiseDetails);
   SetMethodNoSideEffect(context, target, "getProxyDetails", GetProxyDetails);
@@ -413,16 +437,6 @@ void Initialize(Local<Object> target,
 
   SetMethod(
       context, target, "arrayBufferViewHasBuffer", ArrayBufferViewHasBuffer);
-  Local<Object> constants = Object::New(env->isolate());
-  NODE_DEFINE_CONSTANT(constants, ALL_PROPERTIES);
-  NODE_DEFINE_CONSTANT(constants, ONLY_WRITABLE);
-  NODE_DEFINE_CONSTANT(constants, ONLY_ENUMERABLE);
-  NODE_DEFINE_CONSTANT(constants, ONLY_CONFIGURABLE);
-  NODE_DEFINE_CONSTANT(constants, SKIP_STRINGS);
-  NODE_DEFINE_CONSTANT(constants, SKIP_SYMBOLS);
-  target->Set(context,
-              FIXED_ONE_BYTE_STRING(env->isolate(), "propertyFilter"),
-              constants).Check();
 
   Local<String> should_abort_on_uncaught_toggle =
       FIXED_ONE_BYTE_STRING(env->isolate(), "shouldAbortOnUncaughtToggle");