Conversation
mitchute
left a comment
There was a problem hiding this comment.
Code walk through.
| AirLoopMixer *AirLoopMixer::factory(EnergyPlusData &state, int object_num, std::string const &objectName) | ||
| AirLoopMixer *AirLoopMixer::factory(EnergyPlusData &state, const int objectNum, const std::string &objectName) |
There was a problem hiding this comment.
Added const and unified the arg names between the function declaration and implementation.
| std::string const name = Util::makeUPPER(NodeDOASName.at("inlet_node_name").get<std::string>()); | ||
| int const NodeNum = NodeInputManager::GetOnlySingleNode(state, |
There was a problem hiding this comment.
I played with using clang-tidy to automatically detect where const could be added. I'm sure the compiler is likely smart enough to figure all this out.
| for (const auto &thisAirLoopMixerObject : state.dataAirLoopHVACDOAS->airloopMixer) { | ||
| if (Util::SameString(objectName, thisAirLoopMixerObject.name)) { | ||
| index = static_cast<int>(loop); | ||
| return index; |
There was a problem hiding this comment.
Range-based loop conversion.
src/EnergyPlus/AirLoopHVACDOAS.cc
Outdated
| ShowSevereError(state, | ||
| format("{}, \"{}\", {} not found: {}\n", cCurrentModuleObject, thisDOAS.Name, cFieldName, thisDOAS.OASystemName)); | ||
| ShowSevereError( | ||
| state, format(R"({}, "{}", {} not found: {}\n)", cCurrentModuleObject, thisDOAS.Name, cFieldName, thisDOAS.OASystemName)); |
There was a problem hiding this comment.
Maybe these are more readable? I'm not sure whether the \n at the end is needed.
There was a problem hiding this comment.
I don't think I have ever seen a new line used in Show messages. void ShowErrorMessage() already includes the new line.
| if (Util::SameString(state.dataAirLoop->OutsideAirSys(OASysNum).ControllerListName, "")) { | ||
| if (state.dataAirLoop->OutsideAirSys(OASysNum).AirLoopDOASNum == -1) { | ||
| ShowSevereError(state, | ||
| format("AirLoopHVAC:OutdoorAirSystem = \"{}\" invalid Controller List Name = \" not found.", | ||
| format(R"(AirLoopHVAC:OutdoorAirSystem = "{}" invalid Controller List Name = " not found.)", |
There was a problem hiding this comment.
This error message looks odd.
There was a problem hiding this comment.
Also, if there is no coil in the OA system (e.g., just the OA Mixer) then you don't need, and can't even specify (because the sensor node and actuator node are required-fields), a Controller:WaterCoil object. Not sure if a DOAS should ever not have a cooling coil but that cooling coil could be in an AirLoopHVACUnitarySystem or CoilSystemCoolingDX which do not require a controller. I think this should be checking if a controller type coil (heating or cooling) is present before posting this error. Also, the IDD note says the controller list should be blank? and line 872 checks that it's NOT a DOAS (the -1)? Shouldn't this be checking only for controllers for a coil in the DOAS system? in the AirloopHVACDOAS source? Maybe this check got moved here when DOAS was added and this is somehow checking the non-DOAS OA systems but I still think the controller list is optional and only needed if water coils are present. I'm not a DOAS expert but this does look odd in more ways than one.
AirLoopHVAC:OutdoorAirSystem,
\memo Outdoor air subsystem for an AirLoopHVAC. Includes an outdoor air mixing box and
\memo optional outdoor air conditioning equipment such as heat recovery, preheat, and precool
\memo coils. From the perspective of the primary air loop the outdoor air system is treated
\memo as a single component.
\min-fields 3
A1, \field Name
\required-field
\type alpha
\reference-class-name validBranchEquipmentTypes
\reference validBranchEquipmentNames
A2, \field Controller List Name
\note Enter the name of an AirLoopHVAC:ControllerList object or blank if this object is used in
\note AirLoopHVAC:DedicatedOutdoorAirSystem.
There was a problem hiding this comment.
Tagging issue #11291 to keep track of this.
| ShowRecurringWarningErrorAtEnd(state, | ||
| loop.Name + "\": The max difference of node temperatures exceeding 1.0e-6 continues...", | ||
| loop.ConveIndex, | ||
| maxDiff, | ||
| maxDiff); | ||
| ShowRecurringWarningErrorAtEnd( | ||
| state, | ||
| format(R"("{}": The max difference of node temperatures exceeding 1.0e-6 continues...)", loop.Name), | ||
| loop.ConveIndex, | ||
| maxDiff, | ||
| maxDiff); |
There was a problem hiding this comment.
Another odd looking error message. I think I cleaned it up to what was intended.
| @@ -81,14 +78,14 @@ namespace AirLoopHVACDOAS { | |||
| std::vector<int> InletNodeNum; | |||
| Real64 OutletTemp = 0.0; | |||
|
|
|||
| static AirLoopMixer *factory(EnergyPlusData &state, int objectNum, std::string const &objectName); | |||
There was a problem hiding this comment.
Just grouping the functions together, and unifying the arg names.
| } // namespace AirLoopHVACDOAS | ||
|
|
||
| struct AirLoopHVACDOASData : BaseGlobalStruct | ||
| struct AirLoopHVACDOASData final : BaseGlobalStruct |
There was a problem hiding this comment.
What does this do? final?
There was a problem hiding this comment.
It just signals that this struct won't be inherited.
| ValidEquipListType foundType = static_cast<ValidEquipListType>(getEnumValue(validEquipNamesUC, typeNameUC)); | ||
|
|
||
| switch (foundType) { | ||
| switch (static_cast<ValidEquipListType>(getEnumValue(validEquipNamesUC, typeNameUC))) { |
There was a problem hiding this comment.
Smart enough to make this substitution but uses auto in range based loops?
There was a problem hiding this comment.
I think this was me doing stuff manually.
…loop-hvac-cleanups
Pull request overview
I spent a little time looking at clang-tidy as a possible way to do some automated code refactoring. This was mostly a learning exercise for me, but I made a few cleanups (and maybe fixes) along the way, so I figured I'd commit them here.
Description of the purpose of this PR
Pull Request Author
Reviewer