Initial lambda extraction - #566
Conversation
d37b5c3 to
3b7ed08
Compare
8523e16 to
1c5cb77
Compare
|
I am marking this as ready for review, so that we can determine remaining priorities before release. Let me know if this is worth it before release |
nkoukpaizan
left a comment
nkoukpaizan
left a comment
There was a problem hiding this comment.
I like the idea of extracting the lambda. What we need to discuss is where the extracted methods will live.
| std::unique_ptr<MonitorT> monitor_; | ||
|
|
||
| /// Count of parameter-loading errors reported through verify() | ||
| IdxT parameter_error_count_{0}; |
There was a problem hiding this comment.
I wouldn't think we need to store this as a member variable. Removing would also avoid the static_cast<int>.
| template <ExternalVariables variable> | ||
| auto refreshWorkspace(ScalarT fallback, ScalarT* ws, IdxT* ws_indices) const | ||
| { | ||
| static_assert(variable < ExternalVariables::MAXIMUM); | ||
| const auto index = static_cast<size_t>(variable); | ||
| ws[index] = fallback; | ||
| ws_indices[index] = INVALID_INDEX<IdxT>; | ||
| if (isAttached<variable>()) | ||
| { | ||
| ws[index] = readExternalVariable<variable>(); | ||
| ws_indices[index] = readExternalVariableIndex<variable>(); | ||
| } | ||
| } |
There was a problem hiding this comment.
I wouldn't make ComponentSignals depend on ws_. Let's keep these in *Impl.hpp.
| template <ExternalVariables variable> | ||
| auto checkRequired(Utilities::ConfigurationChecks& checks, const char* name) const | ||
| { | ||
| static_assert(variable < ExternalVariables::MAXIMUM); | ||
| if (!isAttached<variable>()) | ||
| { | ||
| checks.fail() << name << " signal is required\n"; | ||
| return; | ||
| } | ||
| if (!isLinked<variable>()) | ||
| { | ||
| checks.fail() << name << " signal attached with no linked source\n"; | ||
| } | ||
| } | ||
|
|
||
| /// Verifies a port that may be absent but must resolve to linked | ||
| /// storage when attached | ||
| /// | ||
| /// @tparam variable The external variable to verify | ||
| /// @param[in,out] checks Error accumulator for the owning component | ||
| /// @param[in] name Port name used in error messages | ||
| template <ExternalVariables variable> | ||
| auto checkOptional(Utilities::ConfigurationChecks& checks, const char* name) const | ||
| { | ||
| static_assert(variable < ExternalVariables::MAXIMUM); | ||
| if (!isAttached<variable>()) | ||
| { | ||
| return; | ||
| } | ||
| if (!isLinked<variable>()) | ||
| { | ||
| checks.fail() << name << " signal attached with no linked source\n"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's not make ComponentSignals depend on ConfigurationChecks. Let's keep these in *Impl.hpp.
| /** | ||
| * @file ConfigurationChecks.hpp | ||
| * @author Luke Lowery (lukel@tamu.edu) | ||
| * @brief Error accumulator for model configuration validation. |
There was a problem hiding this comment.
Documentation will need an update.
| * the `Parameters` enumeration, and the `parameters` map. | ||
| */ | ||
| template <typename ModelDataT> | ||
| class ParameterReader |
There was a problem hiding this comment.
The methods look good to me. I'd like to get thoughts from @PhilipFackler and @superwhiskers on where these should live. I think it should be in GridKit/Model/.
superwhiskers
left a comment
superwhiskers
left a comment
There was a problem hiding this comment.
some quick comments. overall seems like a good change to make
| * @tparam ModelDataT A model data container exposing `RealT`, `IdxT`, | ||
| * the `Parameters` enumeration, and the `parameters` map. | ||
| */ | ||
| template <typename ModelDataT> |
There was a problem hiding this comment.
note: if/when #570 is merged, should probably require the ModelData concept present in GridKit/Model/PhasorDynamics/ModelData.hpp
There was a problem hiding this comment.
you should be able to do this now
| /// Log one error against this model and count it. | ||
| std::ostream& fail() | ||
| { | ||
| ++error_count_; |
There was a problem hiding this comment.
i still think we should move away from counting errors and toward something more robust
d159f08 to
c1aed2d
Compare
bd1a417 to
988eb70
Compare
3 participants
Description
Extract the inconsistent lambdas for parameter validation and signal attachment into one common layer
Proposed changes
Utilities::ConfigurationCheckUtilities::ParameterReaderChecklist
-Wall -Wpedantic -Wconversion -Wextra.Further comments
@nkoukpaizan Drafted until you take a quick look, is this good approach generally?