Restrict some more sim options; Fix Cmake SSL.

* Restrict override.spell_data more drastically.
* Give more info out from armory option parsing. Throw on armory
download without API key, since the "JSON parse fail" is just misleading
here.

CMakeLists.txt

@@ -8,7 +8,7 @@ option(BUILD_GUI "Build the Qt gui along with cli binary" ON)
 set(CMAKE_CXX_STANDARD 11)
 
 # enable colored output with ninja build system
-add_compile_options(-fcolor-diagnostics)
+#add_compile_options(-fcolor-diagnostics)
 
 # output a compile_commands.json used for editor tooling
 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)

engine/CMakeLists.txt

@@ -23,6 +23,7 @@ else()
     if(${OPENSSL_FOUND})
         message(STATUS "Using OpenSSL")
         target_compile_definitions(engine PUBLIC SC_USE_OPENSSL)
+        target_link_libraries(engine ${OPENSSL_LIBRARIES})
     else()
         message(WARNING "No SSL library available. HTTPS access not available.")
     endif()

engine/dbc/dbc.hpp

@@ -411,9 +411,9 @@ namespace dbc_override
     { }
   };
 
-  bool register_effect( dbc_t&, unsigned, const std::string&, double );
-  bool register_spell( dbc_t&, unsigned, const std::string&, double );
-  bool register_power( dbc_t&, unsigned, const std::string&, double );
+  void register_effect( dbc_t&, unsigned, const std::string&, double );
+  void register_spell( dbc_t&, unsigned, const std::string&, double );
+  void register_power( dbc_t&, unsigned, const std::string&, double );
 
   const spell_data_t* find_spell( unsigned, bool ptr = false );
   const spelleffect_data_t* find_effect( unsigned, bool ptr = false );

engine/dbc/sc_data.cpp

@@ -871,7 +871,6 @@ spell_data_t* custom_dbc_data_t::clone_spell( unsigned clone_spell_id, bool ptr
   }
 
   c = get_mutable_spell( clone_spell_id, ptr );
-  assert( c );
   // Return the cloned spell
   return c;
 }
@@ -910,23 +909,22 @@ namespace dbc_override
 }
 
 // Applies overrides immediately, and records an entry
-bool dbc_override::register_spell( dbc_t& dbc, unsigned spell_id, const std::string& field, double v )
+void dbc_override::register_spell( dbc_t& dbc, unsigned spell_id, const std::string& field, double v )
 {
-  spell_data_t* spell = override_db_.get_mutable_spell( spell_id, dbc.ptr );
-  if ( ! spell )
+  spell_data_t* spell = override_db_.clone_spell( spell_id, dbc.ptr );
+  if (!spell)
   {
-    spell = override_db_.clone_spell( spell_id, dbc.ptr );
+    throw std::invalid_argument("Could not find spell");
+  }
+  if (!spell -> override_field( field, v ))
+  {
+    throw std::invalid_argument(fmt::format("Invalid field '{}'.", field));
   }
-
-  assert( spell );
-  spell -> override_field( field, v );
 
   override_entries_.push_back( dbc_override_entry_t( DBC_OVERRIDE_SPELL, field, spell_id, v ) );
-
-  return true;
 }
 
-bool dbc_override::register_effect( dbc_t& dbc, unsigned effect_id, const std::string& field, double v )
+void dbc_override::register_effect( dbc_t& dbc, unsigned effect_id, const std::string& field, double v )
 {
   spelleffect_data_t* effect = override_db_.get_mutable_effect( effect_id, dbc.ptr );
   if ( ! effect )
@@ -935,17 +933,20 @@ bool dbc_override::register_effect( dbc_t& dbc, unsigned effect_id, const std::s
     override_db_.clone_spell( dbc_effect -> spell() -> id(), dbc.ptr );
     effect = override_db_.get_mutable_effect( effect_id, dbc.ptr );
   }
+  if (!effect)
+  {
+    throw std::runtime_error("Could not find effect");
+  }
 
-  assert( effect );
-
-  effect -> override_field( field, v );
+  if (!effect -> override_field( field, v ))
+  {
+    throw std::invalid_argument(fmt::format("Invalid field '{}'.", field));
+  }
 
   override_entries_.push_back( dbc_override_entry_t( DBC_OVERRIDE_EFFECT, field, effect_id, v ) );
-
-  return true;
 }
 
-bool dbc_override::register_power( dbc_t& dbc, unsigned power_id, const std::string& field, double v )
+void dbc_override::register_power( dbc_t& dbc, unsigned power_id, const std::string& field, double v )
 {
   auto power = override_db_.get_mutable_power( power_id, dbc.ptr );
   if ( power == nullptr )
@@ -954,14 +955,17 @@ bool dbc_override::register_power( dbc_t& dbc, unsigned power_id, const std::str
     override_db_.clone_spell( dbc_power -> spell_id(), dbc.ptr );
     power = override_db_.get_mutable_power( power_id, dbc.ptr );
   }
+  if (!power)
+  {
+    throw std::runtime_error("Could not find power");
+  }
 
-  assert( power );
-
-  power -> override_field( field, v );
+  if (!power -> override_field( field, v ))
+  {
+    throw std::invalid_argument(fmt::format("Invalid field '{}'.", field));
+  }
 
   override_entries_.push_back( dbc_override_entry_t( DBC_OVERRIDE_POWER, field, power_id, v ) );
-
-  return true;
 }
 
 const spell_data_t* dbc_override::find_spell( unsigned spell_id, bool ptr )

engine/interfaces/sc_bcp_api.cpp

@@ -807,13 +807,14 @@ player_t* bcp_api::download_player( sim_t*             sim,
   sim -> current_name = name;
 
   player_spec_t player;
+  bool use_new_endpoints = (region != "cn"); // China does not have new api endpoints yet.
 
-  if (!sim -> apikey.empty() && sim -> apikey.size() != 32)
+  if (use_new_endpoints && sim -> apikey.size() != 32)
   {
-    sim -> error( "Check api key, must be 32 characters long." );
+    throw std::runtime_error("No valid api key available. Cannot download from armory. See https://github.com/simulationcraft/simc/wiki/BattleArmoryAPI" );
   }
 
-  if ( sim -> apikey.size() == 32 && region != "cn" ) // China does not have new api endpoints yet.
+  if ( use_new_endpoints )
   {
     std::string battlenet = "https://" + region + ".api.battle.net/";
 

engine/sim/sc_sim.cpp

@@ -385,7 +385,7 @@ class names_and_options_t
     {
       if ( names[ i ].find( '=' ) != std::string::npos )
       {
-        opts::parse( sim, context.c_str(), options, names[ i ] );
+        opts::parse( sim, context, options, names[ i ] );
       }
       else
       {
@@ -398,7 +398,7 @@ class names_and_options_t
 
     if ( region.empty() )
     {
-      if ( names.size() > 2 && is_valid_region( names[ 0 ] ) )
+      if ( names.size() > 2 )
       {
         region = names[ 0 ];
         server = names[ 1 ];
@@ -409,6 +409,11 @@ class names_and_options_t
         region = sim -> default_region_str;
       }
     }
+    if (!is_valid_region( region ))
+    {
+      throw std::invalid_argument(fmt::format("Invalid region '{}'.", region));
+    }
+
     if ( server.empty() )
     {
       if ( names.size() > 1 )
@@ -430,8 +435,6 @@ bool parse_armory( sim_t*             sim,
                    const std::string& name,
                    const std::string& value )
 {
-  try
-  {
     std::string spec = "active";
 
     std::vector<std::unique_ptr<option_t>> options;
@@ -478,10 +481,6 @@ bool parse_armory( sim_t*             sim,
         std::throw_with_nested(std::runtime_error("BCP API"));
       }
     }
-  }
-
-  catch ( names_and_options_t::error& )
-  { return false; }
 
   // Create options for player
   if ( sim -> active_player )
@@ -683,47 +682,44 @@ bool parse_override_spell_data( sim_t*             sim,
   size_t v_pos = value.find( '=' );
 
   if ( v_pos == std::string::npos )
-    return false;
+  {
+    throw std::invalid_argument("Invalid form. Spell data override takes the form <spell|effect|power>.<id>.<field>=value");
+  }
 
   std::vector< std::string > splits = util::string_split( value.substr( 0, v_pos ), "." );
 
   if ( splits.size() != 3 )
-    return false;
+  {
+    throw std::invalid_argument("Invalid form. Spell data override takes the form <spell|effect|power>.<id>.<field>=value");
+  }
 
-  unsigned long int id = strtoul( splits[ 1 ].c_str(), nullptr, 10 );
-  if ( id == 0 || id == std::numeric_limits<unsigned long>::max() )
-    return false;
+  int parsed_id = std::stoi( splits[ 1 ] );
+  if ( parsed_id <= 0 )
+  {
+    throw std::invalid_argument("Invalid spell id (negative or zero).");
+  }
+  unsigned id = as<unsigned>(parsed_id);
 
-  double v = strtod( value.substr( v_pos + 1 ).c_str(), nullptr );
-  if ( v == std::numeric_limits<double>::min() || v == std::numeric_limits<double>::max() )
-    return false;
+  double v = std::stod( value.substr( v_pos + 1 ) );
 
   if ( util::str_compare_ci( splits[ 0 ], "spell" ) )
   {
-    if ( ! dbc_override::register_spell( sim -> dbc, id, splits[ 2 ], v ) )
-    {
-      return false;
-    }
-    return true;
+    dbc_override::register_spell( sim -> dbc, id, splits[ 2 ], v );
   }
   else if ( util::str_compare_ci( splits[ 0 ], "effect" ) )
   {
-    if ( ! dbc_override::register_effect( sim -> dbc, id, splits[ 2 ], v ) )
-    {
-      return false;
-    }
-    return true;
+    dbc_override::register_effect( sim -> dbc, id, splits[ 2 ], v );
   }
   else if ( util::str_compare_ci( splits[ 0 ], "power" ) )
   {
-    if ( ! dbc_override::register_power( sim -> dbc, id, splits[ 2 ], v ) )
-    {
-      return false;
-    }
-    return true;
+    dbc_override::register_power( sim -> dbc, id, splits[ 2 ], v );
   }
   else
-    return false;
+  {
+    throw std::invalid_argument("Invalid form. Spell data override takes the form <spell|effect|power>.<id>.<field>=value");
+  }
+
+  return true;
 }
 
 // parse_override_target_health =============================================
@@ -827,9 +823,8 @@ bool parse_item_sources( sim_t*             sim,
       all_known_sources += default_item_db_sources[ i ];
     }
 
-    sim -> errorf( "Your global data source string \"%s\" contained no valid data sources. Valid identifiers are:%s",
-                   value.c_str(), all_known_sources.c_str() );
-    return false;
+    throw std::invalid_argument(fmt::format("Your global data source string '{}' contained no valid data sources. "
+        "Valid identifiers are: {}", value, all_known_sources ));
   }
 
   return true;
@@ -877,6 +872,11 @@ bool parse_target_error_role( sim_t * sim,
 {
   sim -> target_error_role = util::parse_role_type( value );
 
+  if (sim -> target_error_role == ROLE_NONE)
+  {
+    throw std::invalid_argument("Invalid target error role");
+  }
+
   return true;
 }
 
@@ -886,8 +886,7 @@ bool parse_maximize_reporting( sim_t*             sim,
 {
   if ( v != "0" && v != "1" )
   {
-    sim -> errorf( "Acceptable values for '%s' are '1' or '0'\n", name.c_str() );
-    return false;
+    throw std::invalid_argument("Acceptable values are '1' or '0'.");
   }
   bool r = std::stoi( v ) != 0;
   if ( r )
@@ -1187,7 +1186,7 @@ std::string get_api_key()
     }
     else
     {
-      std::cerr << "Blizzard API Key '" << line << "' from file '" << filename << "' was not properly entered." << std::endl;
+      std::cerr << "Blizzard API Key from file '" << filename << "' was not properly entered. (Size != 32)" << std::endl;
     }
   }