Adjust and cleanup per reviewer comments

Author: lukelowry

GridKit/Model/ConfigurationChecks.hpp

@@ -22,7 +22,7 @@ namespace GridKit
      * condition that does not hold and returns it. The caller decides how
      * the messages are reported; passed() is true when there are none.
      */
-    class ConfigurationChecks
+    class [[nodiscard]] ConfigurationChecks
     {
     public:
       ConfigurationChecks() = default;

GridKit/Model/ParameterReader.hpp

@@ -8,6 +8,7 @@
 
 #include <cmath>
 #include <format>
+#include <optional>
 #include <stdexcept>
 #include <string_view>
 #include <variant>
@@ -24,12 +25,11 @@ namespace GridKit
     /**
      * @brief Reads typed parameters out of a model data container.
      *
-     * A load method stores a provided value in its target and returns true.
-     * An omitted parameter leaves the target untouched, so the target's
-     * initial value is the default. A require method rejects an omitted
-     * parameter. A value of the wrong type cannot produce a valid model, so
-     * it is rejected with std::invalid_argument naming the model and the
-     * parameter. Real parameters accept integer values and must be finite.
+     * A read method returns the provided value or std::nullopt when the
+     * parameter is omitted. The model chooses a default or requires a value.
+     * Invalid values are rejected with std::invalid_argument naming the
+     * model and parameter. Real parameters accept integer values and must
+     * be finite.
      *
      * @tparam ModelDataT A model data container satisfying
      *         PhasorDynamics::ModelData.
@@ -52,13 +52,13 @@ namespace GridKit
       {
       }
 
-      /// Load an optional real parameter.
-      bool loadReal(ParametersT key, RealT& target) const
+      /// Read an optional real parameter.
+      [[nodiscard]] std::optional<RealT> readReal(ParametersT key) const
       {
         const auto* value = find(key);
         if (value == nullptr)
         {
-          return false;
+          return std::nullopt;
         }
 
         RealT real{};
@@ -80,57 +80,30 @@ namespace GridKit
           reject(key, "must be finite");
         }
 
-        target = real;
-        return true;
+        return real;
       }
 
-      /// Load an optional non-negative real parameter.
-      bool loadNonNegative(ParametersT key, RealT& target) const
+      /// Read an optional non-negative real parameter.
+      [[nodiscard]] std::optional<RealT> readNonNegative(ParametersT key) const
       {
-        if (!loadReal(key, target))
-        {
-          return false;
-        }
-
-        if (target < ZERO<RealT>)
+        auto value = readReal(key);
+        if (value && *value < ZERO<RealT>)
         {
           reject(key, "must be non-negative");
         }
-        return true;
-      }
-
-      /// Load an optional boolean parameter.
-      bool loadBool(ParametersT key, bool& target) const
-      {
-        return loadExact(key, target, "must be boolean");
-      }
-
-      /// Load an optional integer parameter.
-      bool loadInteger(ParametersT key, IdxT& target) const
-      {
-        return loadExact(key, target, "must be an integer");
+        return value;
       }
 
-      /// Read a real parameter that must be provided.
-      RealT requireReal(ParametersT key) const
+      /// Read an optional boolean parameter.
+      [[nodiscard]] std::optional<bool> readBool(ParametersT key) const
       {
-        RealT real{};
-        if (!loadReal(key, real))
-        {
-          reject(key, "is required");
-        }
-        return real;
+        return readExact<bool>(key, "must be boolean");
       }
 
-      /// Read a boolean parameter that must be provided.
-      bool requireBool(ParametersT key) const
+      /// Read an optional integer parameter.
+      [[nodiscard]] std::optional<IdxT> readInteger(ParametersT key) const
       {
-        bool flag{};
-        if (!loadBool(key, flag))
-        {
-          reject(key, "is required");
-        }
-        return flag;
+        return readExact<IdxT>(key, "must be an integer");
       }
 
     private:
@@ -148,14 +121,14 @@ namespace GridKit
         return &entry->second;
       }
 
-      /// Load an optional parameter that must hold exactly a T.
+      /// Read an optional parameter that must hold exactly a T.
       template <typename T>
-      bool loadExact(ParametersT key, T& target, std::string_view reason) const
+      [[nodiscard]] std::optional<T> readExact(ParametersT key, std::string_view reason) const
       {
         const auto* value = find(key);
         if (value == nullptr)
         {
-          return false;
+          return std::nullopt;
         }
 
         const auto* exact_value = std::get_if<T>(value);
@@ -164,8 +137,7 @@ namespace GridKit
           reject(key, reason);
         }
 
-        target = *exact_value;
-        return true;
+        return *exact_value;
       }
 
       /// Reject the model data with a message naming the model and parameter.

GridKit/Model/PhasorDynamics/Component.hpp

@@ -62,8 +62,11 @@ namespace GridKit
         }
       }
 
-      /// Report every configuration problem; passed() when the model can initialize.
-      virtual Model::ConfigurationChecks verify() const = 0;
+      /// Report configuration problems. Override when the component has checks.
+      virtual Model::ConfigurationChecks verify() const
+      {
+        return {};
+      }
 
       IdxT size() override final
       {
@@ -287,7 +290,7 @@ namespace GridKit
        *
        * @return true when the configuration has no problems.
        */
-      bool verifyAndLog() const
+      [[nodiscard]] bool verifyAndLog() const
       {
         const auto checks = verify();
         for (const auto& error : checks.errors())

GridKit/Model/PhasorDynamics/Exciter/ESDC1A/Esdc1aImpl.hpp

@@ -636,26 +636,26 @@ namespace GridKit
       {
         using Params = typename ModelDataT::Parameters;
 
-        Model::ParameterReader reader(data, "Esdc1a");
-
-        reader.loadNonNegative(Params::Tr, Tr_);
-        reader.loadReal(Params::Ka, Ka_);
-        reader.loadNonNegative(Params::Ta, Ta_);
-        reader.loadNonNegative(Params::Tb, Tb_);
-        reader.loadReal(Params::Tc, Tc_);
-        reader.loadReal(Params::Vrmax, Vrmax_);
-        reader.loadReal(Params::Vrmin, Vrmin_);
-        reader.loadReal(Params::Ke, Ke_);
-        reader.loadNonNegative(Params::Te, Te_);
-        reader.loadReal(Params::Kf, Kf_);
-        reader.loadNonNegative(Params::Tf1, Tf1_);
-        reader.loadBool(Params::Spdmlt, Spdmlt_);
-        reader.loadReal(Params::E1, E1_);
-        reader.loadReal(Params::Se1, Se1_);
-        reader.loadReal(Params::E2, E2_);
-        reader.loadReal(Params::Se2, Se2_);
-        reader.loadInteger(Params::UEL, UEL_);
-        reader.loadBool(Params::exclim, exclim_);
+        const Model::ParameterReader reader(data, "Esdc1a");
+
+        Tr_     = reader.readNonNegative(Params::Tr).value_or(Tr_);
+        Ka_     = reader.readReal(Params::Ka).value_or(Ka_);
+        Ta_     = reader.readNonNegative(Params::Ta).value_or(Ta_);
+        Tb_     = reader.readNonNegative(Params::Tb).value_or(Tb_);
+        Tc_     = reader.readReal(Params::Tc).value_or(Tc_);
+        Vrmax_  = reader.readReal(Params::Vrmax).value_or(Vrmax_);
+        Vrmin_  = reader.readReal(Params::Vrmin).value_or(Vrmin_);
+        Ke_     = reader.readReal(Params::Ke).value_or(Ke_);
+        Te_     = reader.readNonNegative(Params::Te).value_or(Te_);
+        Kf_     = reader.readReal(Params::Kf).value_or(Kf_);
+        Tf1_    = reader.readNonNegative(Params::Tf1).value_or(Tf1_);
+        Spdmlt_ = reader.readBool(Params::Spdmlt).value_or(Spdmlt_);
+        E1_     = reader.readReal(Params::E1).value_or(E1_);
+        Se1_    = reader.readReal(Params::Se1).value_or(Se1_);
+        E2_     = reader.readReal(Params::E2).value_or(E2_);
+        Se2_    = reader.readReal(Params::Se2).value_or(Se2_);
+        UEL_    = reader.readInteger(Params::UEL).value_or(UEL_);
+        exclim_ = reader.readBool(Params::exclim).value_or(exclim_);
         setDerivedParameters();
       }
 

GridKit/Model/PhasorDynamics/Load/LoadZ/LoadZ.hpp

@@ -56,11 +56,6 @@ namespace GridKit
       virtual int evaluateResidual() override final;
       virtual int evaluateJacobian() override final;
 
-      virtual Model::ConfigurationChecks verify() const override final
-      {
-        return {};
-      }
-
     public:
       void setR(RealT R)
       {

GridKit/Model/PhasorDynamics/Load/LoadZIP/LoadZIP.hpp

@@ -56,11 +56,6 @@ namespace GridKit
       int evaluateResidual() override final;
       int evaluateJacobian() override final;
 
-      Model::ConfigurationChecks verify() const override final
-      {
-        return {};
-      }
-
     public:
       void setPnom(RealT Pnom)
       {

GridKit/Model/PhasorDynamics/SignalSource/ConstantSignalSource.hpp

@@ -51,8 +51,6 @@ namespace GridKit
       int evaluateResidual() override final;
       int evaluateJacobian() override final;
 
-      Model::ConfigurationChecks verify() const override final;
-
       SignalPortsT& getPorts()
       {
         return ports_;

GridKit/Model/PhasorDynamics/SignalSource/ConstantSignalSourceImpl.hpp

@@ -83,12 +83,6 @@ namespace GridKit
       return 0;
     }
 
-    template <typename scalar_type, typename index_type>
-    Model::ConfigurationChecks ConstantSignalSource<scalar_type, index_type>::verify() const
-    {
-      return {};
-    }
-
     template <typename scalar_type, typename index_type>
     int ConstantSignalSource<scalar_type, index_type>::initialize()
     {

GridKit/Model/PhasorDynamics/SignalSource/FunctionSignalSource.hpp

@@ -54,8 +54,6 @@ namespace GridKit
       int evaluateResidual() override final;
       int evaluateJacobian() override final;
 
-      Model::ConfigurationChecks verify() const override final;
-
       void updateTime(RealT t, RealT a) override;
 
       SignalPortsT& getPorts()

GridKit/Model/PhasorDynamics/SignalSource/FunctionSignalSourceImpl.hpp

@@ -109,12 +109,6 @@ namespace GridKit
       return 0;
     }
 
-    template <typename scalar_type, typename index_type>
-    Model::ConfigurationChecks FunctionSignalSource<scalar_type, index_type>::verify() const
-    {
-      return {};
-    }
-
     template <typename scalar_type, typename index_type>
     int FunctionSignalSource<scalar_type, index_type>::initialize()
     {