src: refactor node options parsers to mitigate MSVC bug

src/node.cc

@@ -546,7 +546,7 @@ int ProcessGlobalArgs(std::vector<std::string>* args,
   std::vector<std::string> v8_args;
 
   Mutex::ScopedLock lock(per_process::cli_options_mutex);
-  options_parser::PerProcessOptionsParser::instance.Parse(
+  options_parser::Parse(
       args,
       exec_args,
       &v8_args,

src/node_config.cc

@@ -1,7 +1,7 @@
 #include "env-inl.h"
 #include "node.h"
 #include "node_i18n.h"
-#include "node_options-inl.h"
+#include "node_options.h"
 #include "util-inl.h"
 
 namespace node {

src/node_options-inl.h

@@ -213,15 +213,15 @@ auto OptionsParser<Options>::Convert(
 template <typename Options>
 template <typename ChildOptions>
 void OptionsParser<Options>::Insert(
-    const OptionsParser<ChildOptions>* child_options_parser,
+    const OptionsParser<ChildOptions>& child_options_parser,
     ChildOptions* (Options::* get_child)()) {
-  aliases_.insert(child_options_parser->aliases_.begin(),
-                  child_options_parser->aliases_.end());
+  aliases_.insert(std::begin(child_options_parser.aliases_),
+                  std::end(child_options_parser.aliases_));
 
-  for (const auto& pair : child_options_parser->options_)
+  for (const auto& pair : child_options_parser.options_)
     options_.emplace(pair.first, Convert(pair.second, get_child));
 
-  for (const auto& pair : child_options_parser->implications_)
+  for (const auto& pair : child_options_parser.implications_)
     implications_.emplace(pair.first, Convert(pair.second, get_child));
 }
 

src/node_options.cc

@@ -1,8 +1,9 @@
-#include <cerrno>
 #include "env-inl.h"
 #include "node_binding.h"
 #include "node_options-inl.h"
 
+#include <cstdlib>  // strtoul, errno
+
 using v8::Boolean;
 using v8::Context;
 using v8::FunctionCallbackInfo;
@@ -113,19 +114,60 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
 
 namespace options_parser {
 
-// Explicitly access the singelton instances in their dependancy order.
-// This was moved here to workaround a compiler bug.
-// Refs: https://github.com/nodejs/node/issues/25593
+class DebugOptionsParser : public OptionsParser<DebugOptions> {
+ public:
+  DebugOptionsParser();
+};
+
+class EnvironmentOptionsParser : public OptionsParser<EnvironmentOptions> {
+ public:
+  EnvironmentOptionsParser();
+  explicit EnvironmentOptionsParser(const DebugOptionsParser& dop)
+    : EnvironmentOptionsParser() {
+    Insert(dop, &EnvironmentOptions::get_debug_options);
+  }
+};
 
-#if HAVE_INSPECTOR
-const DebugOptionsParser DebugOptionsParser::instance;
-#endif  // HAVE_INSPECTOR
+class PerIsolateOptionsParser : public OptionsParser<PerIsolateOptions> {
+ public:
+  PerIsolateOptionsParser() = delete;
+  explicit PerIsolateOptionsParser(const EnvironmentOptionsParser& eop);
+};
 
-const EnvironmentOptionsParser EnvironmentOptionsParser::instance;
+class PerProcessOptionsParser : public OptionsParser<PerProcessOptions> {
+ public:
+  PerProcessOptionsParser() = delete;
+  explicit PerProcessOptionsParser(const PerIsolateOptionsParser& iop);
+};
 
-const PerIsolateOptionsParser PerIsolateOptionsParser::instance;
+#if HAVE_INSPECTOR
+const DebugOptionsParser _dop_instance{};
+const EnvironmentOptionsParser _eop_instance{_dop_instance};
+#else
+const EnvironmentOptionsParser _eop_instance{};
+#endif  // HAVE_INSPECTOR
+const PerIsolateOptionsParser _piop_instance{_eop_instance};
+const PerProcessOptionsParser _ppop_instance{_piop_instance};
+
+template <>
+void Parse(
+  StringVector* const args, StringVector* const exec_args,
+  StringVector* const v8_args,
+  PerIsolateOptions* const options,
+  OptionEnvvarSettings required_env_settings, StringVector* const errors) {
+  _piop_instance.Parse(
+    args, exec_args, v8_args, options, required_env_settings, errors);
+}
 
-const PerProcessOptionsParser PerProcessOptionsParser::instance;
+template <>
+void Parse(
+  StringVector* const args, StringVector* const exec_args,
+  StringVector* const v8_args,
+  PerProcessOptions* const options,
+  OptionEnvvarSettings required_env_settings, StringVector* const errors) {
+  _ppop_instance.Parse(
+    args, exec_args, v8_args, options, required_env_settings, errors);
+}
 
 // XXX: If you add an option here, please also add it to doc/node.1 and
 // doc/api/cli.md
@@ -291,14 +333,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             &EnvironmentOptions::tls_v1_1,
             kAllowedInEnvironment);
 #endif
-
-#if HAVE_INSPECTOR
-  Insert(&DebugOptionsParser::instance,
-         &EnvironmentOptions::get_debug_options);
-#endif  // HAVE_INSPECTOR
 }
 
-PerIsolateOptionsParser::PerIsolateOptionsParser() {
+PerIsolateOptionsParser::PerIsolateOptionsParser(
+  const EnvironmentOptionsParser& eop) {
   AddOption("--track-heap-objects",
             "track heap object allocations for heap snapshots",
             &PerIsolateOptions::track_heap_objects,
@@ -354,11 +392,11 @@ PerIsolateOptionsParser::PerIsolateOptionsParser() {
             kAllowedInEnvironment);
 #endif  // NODE_REPORT
 
-  Insert(&EnvironmentOptionsParser::instance,
-         &PerIsolateOptions::get_per_env_options);
+  Insert(eop, &PerIsolateOptions::get_per_env_options);
 }
 
-PerProcessOptionsParser::PerProcessOptionsParser() {
+PerProcessOptionsParser::PerProcessOptionsParser(
+  const PerIsolateOptionsParser& iop) {
   AddOption("--title",
             "the process title to use on startup",
             &PerProcessOptions::title,
@@ -464,8 +502,7 @@ PerProcessOptionsParser::PerProcessOptionsParser() {
 #endif
 #endif
 
-  Insert(&PerIsolateOptionsParser::instance,
-         &PerProcessOptions::get_per_isolate_options);
+  Insert(iop, &PerProcessOptions::get_per_isolate_options);
 }
 
 inline std::string RemoveBrackets(const std::string& host) {
@@ -479,7 +516,8 @@ inline int ParseAndValidatePort(const std::string& port,
                                 std::vector<std::string>* errors) {
   char* endptr;
   errno = 0;
-  const long result = strtol(port.c_str(), &endptr, 10);  // NOLINT(runtime/int)
+  const unsigned long result =                 // NOLINT(runtime/int)
+    strtoul(port.c_str(), &endptr, 10);
   if (errno != 0 || *endptr != '\0'||
       (result != 0 && result < 1024) || result > 65535) {
     errors->push_back(" must be 0 or in range 1024 to 65535.");
@@ -531,10 +569,8 @@ void GetOptions(const FunctionCallbackInfo<Value>& args) {
     per_process::cli_options->per_isolate = original_per_isolate;
   });
 
-  const auto& parser = PerProcessOptionsParser::instance;
-
   Local<Map> options = Map::New(isolate);
-  for (const auto& item : parser.options_) {
+  for (const auto& item : _ppop_instance.options_) {
     Local<Value> value;
     const auto& option_info = item.second;
     auto field = option_info.field;
@@ -552,29 +588,34 @@ void GetOptions(const FunctionCallbackInfo<Value>& args) {
         }
         break;
       case kBoolean:
-        value = Boolean::New(isolate, *parser.Lookup<bool>(field, opts));
+        value = Boolean::New(isolate,
+                             *_ppop_instance.Lookup<bool>(field, opts));
         break;
       case kInteger:
-        value = Number::New(isolate, *parser.Lookup<int64_t>(field, opts));
+        value = Number::New(isolate,
+                            *_ppop_instance.Lookup<int64_t>(field, opts));
         break;
       case kUInteger:
-        value = Number::New(isolate, *parser.Lookup<uint64_t>(field, opts));
+        value = Number::New(isolate,
+                            *_ppop_instance.Lookup<uint64_t>(field, opts));
         break;
       case kString:
-        if (!ToV8Value(context, *parser.Lookup<std::string>(field, opts))
+        if (!ToV8Value(context,
+                       *_ppop_instance.Lookup<std::string>(field, opts))
                  .ToLocal(&value)) {
           return;
         }
         break;
       case kStringList:
         if (!ToV8Value(context,
-                       *parser.Lookup<std::vector<std::string>>(field, opts))
+                       *_ppop_instance.Lookup<StringVector>(field, opts))
                  .ToLocal(&value)) {
           return;
         }
         break;
       case kHostPort: {
-        const HostPort& host_port = *parser.Lookup<HostPort>(field, opts);
+        const HostPort& host_port =
+          *_ppop_instance.Lookup<HostPort>(field, opts);
         Local<Object> obj = Object::New(isolate);
         Local<Value> host;
         if (!ToV8Value(context, host_port.host()).ToLocal(&host) ||
@@ -615,7 +656,7 @@ void GetOptions(const FunctionCallbackInfo<Value>& args) {
   }
 
   Local<Value> aliases;
-  if (!ToV8Value(context, parser.aliases_).ToLocal(&aliases)) return;
+  if (!ToV8Value(context, _ppop_instance.aliases_).ToLocal(&aliases)) return;
 
   Local<Object> ret = Object::New(isolate);
   if (ret->Set(context, env->options_string(), options).IsNothing() ||

src/node_options.h

@@ -290,7 +290,7 @@ class OptionsParser {
   // a method that yields the target options type from this parser's options
   // type.
   template <typename ChildOptions>
-  void Insert(const OptionsParser<ChildOptions>* child_options_parser,
+  void Insert(const OptionsParser<ChildOptions>& child_options_parser,
               ChildOptions* (Options::* get_child)());
 
   // Parse a sequence of options into an options struct, a list of
@@ -310,12 +310,12 @@ class OptionsParser {
   //
   // If `*error` is set, the result of the parsing should be discarded and the
   // contents of any of the argument vectors should be considered undefined.
-  virtual void Parse(std::vector<std::string>* const args,
-                     std::vector<std::string>* const exec_args,
-                     std::vector<std::string>* const v8_args,
-                     Options* const options,
-                     OptionEnvvarSettings required_env_settings,
-                     std::vector<std::string>* const errors) const;
+  void Parse(std::vector<std::string>* const args,
+             std::vector<std::string>* const exec_args,
+             std::vector<std::string>* const v8_args,
+             Options* const options,
+             OptionEnvvarSettings required_env_settings,
+             std::vector<std::string>* const errors) const;
 
  private:
   // We support the wide variety of different option types by remembering
@@ -396,33 +396,12 @@ class OptionsParser {
   friend void GetOptions(const v8::FunctionCallbackInfo<v8::Value>& args);
 };
 
-class DebugOptionsParser : public OptionsParser<DebugOptions> {
- public:
-  DebugOptionsParser();
-
-  static const DebugOptionsParser instance;
-};
-
-class EnvironmentOptionsParser : public OptionsParser<EnvironmentOptions> {
- public:
-  EnvironmentOptionsParser();
-
-  static const EnvironmentOptionsParser instance;
-};
-
-class PerIsolateOptionsParser : public OptionsParser<PerIsolateOptions> {
- public:
-  PerIsolateOptionsParser();
-
-  static const PerIsolateOptionsParser instance;
-};
-
-class PerProcessOptionsParser : public OptionsParser<PerProcessOptions> {
- public:
-  PerProcessOptionsParser();
-
-  static const PerProcessOptionsParser instance;
-};
+using StringVector = std::vector<std::string>;
+template <class OptionsType, class = Options>
+void Parse(
+  StringVector* const args, StringVector* const exec_args,
+  StringVector* const v8_args, OptionsType* const options,
+  OptionEnvvarSettings required_env_settings, StringVector* const errors);
 
 }  // namespace options_parser
 

src/node_process_object.cc

@@ -1,5 +1,3 @@
-#include <climits>  // PATH_MAX
-
 #include "env-inl.h"
 #include "node_internals.h"
 #include "node_options-inl.h"
@@ -8,6 +6,8 @@
 #include "node_revert.h"
 #include "util-inl.h"
 
+#include <climits>  // PATH_MAX
+
 namespace node {
 using v8::Context;
 using v8::DEFAULT;

src/node_worker.cc

@@ -476,7 +476,7 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
 
       // Using invalid_args as the v8_args argument as it stores unknown
       // options for the per isolate parser.
-      options_parser::PerIsolateOptionsParser::instance.Parse(
+      options_parser::Parse(
           &exec_argv,
           &exec_argv_out,
           &invalid_args,
@@ -487,13 +487,13 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
       // The first argument is program name.
       invalid_args.erase(invalid_args.begin());
       if (errors.size() > 0 || invalid_args.size() > 0) {
-        v8::Local<v8::Value> value =
+        v8::Local<v8::Value> error =
             ToV8Value(env->context(),
                       errors.size() > 0 ? errors : invalid_args)
                 .ToLocalChecked();
         Local<String> key =
             FIXED_ONE_BYTE_STRING(env->isolate(), "invalidExecArgv");
-        args.This()->Set(env->context(), key, value).FromJust();
+        USE(args.This()->Set(env->context(), key, error).FromJust());
         return;
       }
     }