CppCheck ZoneEquipmentManager #10705

rraustad · 2024-08-31T04:50:13Z

Pull request overview

Fixes programming syntax
Use Cppcheck as guide

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Reviewer

This will not be exhaustively relevant to every PR.

rraustad · 2024-08-31T04:58:59Z

src/EnergyPlus/ZoneEquipmentManager.cc

@@ -1758,7 +1757,7 @@ void updateZoneSizingEndDay(DataSizing::ZoneSizingData &zsCalcSizing,
    // save heat peak conditions when there is no design heating load or design heating volume flow rate, i.e., when
    // zone temperature is always greater than the zone heating thermostat temperature
    if (zsCalcFinalSizing.DesHeatLoad == 0) {
-        bool FirstIteration = true;
+        bool FirstIteration = true; // declare as static to save for next iteration? but needs to be space/zone specific?


Move to struct to get expected result? It seems the intent is to set these sizing data on a first pass but is not working as intended.

[src/EnergyPlus/ZoneEquipmentManager.cc:1791]:(style),[unreadVariable],Variable 'FirstIteration' is assigned a value that is never used.

If I'm following correctly, this is being used locally here to initialize the peak data to the first timestep values, then let the < (or >) comparisons take over for the remaining timesteps in the loop. So, this really is very local, and it is used in the IF, so not sure why cppCheck thinks it isn't used. The name is misleading, could be firstTimeStep or just delete it and put || TimeStepIndex = 1 in the if.

Looking at this again. This is in EndDay calcs so it seems each of these blocks should be executed at least once. That does not appear to be happening for the later blocks where a previous block was executed and the later block doesn't set bool FirstIteration = true;. I think the || TimeStepIndex == 1 is the better approach. I expect diffs so will target this section in a new branch. I have seen hour of peak = 24 (usually a heating design day) in the past so wondering if TimeStepIndex ==1 or numTimeStepInDay (with condition to not overwrite if set on TimeStepIndex < numTimeStepInDay) is better? If there is no load or flow the entire day maybe it doesn't matter so will try with TimeStepIndex ==1 to see what happens.

Oh, and CppCheck thinks FirstIteration = false; at line 1791 is never used.

rraustad · 2024-08-31T05:01:02Z

src/EnergyPlus/ZoneEquipmentManager.cc

-    bool heatDDNumAllSame = true;
-    bool coolDDNumAllSame = true;
-    int priorHeatDDNum = 0;
-    int priorCoolDDNum = 0;


[src/EnergyPlus/ZoneEquipmentManager.cc:1989]:(style),[unreadVariable],Variable 'heatDDNumAllSame' is assigned a value that is never used.
[src/EnergyPlus/ZoneEquipmentManager.cc:1990]:(style),[unreadVariable],Variable 'coolDDNumAllSame' is assigned a value that is never used.
[src/EnergyPlus/ZoneEquipmentManager.cc:1991]:(style),[unreadVariable],Variable 'priorHeatDDNum' is assigned a value that is never used.
[src/EnergyPlus/ZoneEquipmentManager.cc:1992]:(style),[unreadVariable],Variable 'priorCoolDDNum' is assigned a value that is never used.

Hmm, oh these were left over from plan A, but changed to plan B. Ok.

rraustad · 2024-08-31T05:01:36Z

src/EnergyPlus/ZoneEquipmentManager.cc

@@ -1004,7 +1004,6 @@ void SetUpZoneSizingArrays(EnergyPlusData &state)
            // If this is a DesignSpecification:OutdoorAir:SpaceList check to make sure spaces are valid and belong to this zone
            if (thisOAReq.numDSOA > 0) {
                for (int spaceCounter = 1; spaceCounter <= thisOAReq.numDSOA; ++spaceCounter) {
-                    std::string thisSpaceName = thisOAReq.dsoaSpaceNames(spaceCounter);


[src/EnergyPlus/ZoneEquipmentManager.cc:1007]:(style),[unreadVariable],Variable 'thisSpaceName' is assigned a value that is never used.

rraustad · 2024-08-31T05:02:07Z

src/EnergyPlus/ZoneEquipmentManager.cc

@@ -315,8 +315,6 @@ void sizeZoneSpaceEquipmentPart1(EnergyPlusData &state,
                                                : state.dataZoneTempPredictorCorrector->zoneHeatBalance(zoneNum).NonAirSystemResponse;
    auto &sysDepZoneLoads = (spaceNum > 0) ? state.dataZoneTempPredictorCorrector->spaceHeatBalance(spaceNum).SysDepZoneLoads
                                           : state.dataZoneTempPredictorCorrector->zoneHeatBalance(zoneNum).SysDepZoneLoads;
-    auto &zoneLatentGain = (spaceNum > 0) ? state.dataZoneTempPredictorCorrector->spaceHeatBalance(spaceNum).latentGain
-                                          : state.dataZoneTempPredictorCorrector->zoneHeatBalance(zoneNum).latentGain;


[src/EnergyPlus/ZoneEquipmentManager.cc:318]:(style),[variableScope],The scope of the variable 'zoneLatentGain' can be reduced.

@mjwitte this is 1 thing causing a conflict in this branch. It looks like you did something different with zoneLatentGain maybe? I'm hesitant to smash in a conflict resolution when there may be side effects.

This can go all the way at the bottom of the function inside this:

if (zsCalcSizing.zoneLatentSizing) { int ZoneMult = zoneOrSpace.Multiplier * zoneOrSpace.ListMultiplier; auto &zoneLatentGain = (spaceNum > 0) ? state.dataZoneTempPredictorCorrector>spaceHeatBalance(spaceNum).latentGain : state.dataZoneTempPredictorCorrector->zoneHeatBalance(zoneNum).latentGain; zoneLatentGain += (LatOutputProvided * HgAir) / ZoneMult; }

rraustad · 2024-08-31T05:02:34Z

src/EnergyPlus/ZoneEquipmentManager.cc

@@ -558,13 +556,15 @@ void sizeZoneSpaceEquipmentPart1(EnergyPlusData &state,
    } else {
        nonAirSystemResponse = SysOutputProvided;
        if (state.dataHeatBal->doSpaceHeatBalance) {
-            for (int spaceNum : state.dataHeatBal->Zone(zoneNum).spaceIndexes) {
+            for (int spaceNumLoop : state.dataHeatBal->Zone(zoneNum).spaceIndexes) {


[src/EnergyPlus/ZoneEquipmentManager.cc:561]:(style),[shadowArgument],Local variable 'spaceNum' shadows outer argument

This is the other thing causing a conflict. Obviously this is minor, and it looks like you also changed this to be spaceNum2 or something.

Yes, you can keep either one spaceNum2 or spaceNumLoop. I'll look at the other conflict.

rraustad · 2024-09-03T14:07:09Z

src/EnergyPlus/ZoneEquipmentManager.cc

-                for (std::size_t i = 0; i < state.dataSize->FinalSpaceSizing.size(); ++i) {
-                    updateZoneSizingEndZoneSizingCalc5(state.dataSize->FinalSpaceSizing[i], state.dataSize->CalcFinalSpaceSizing[i]);
+                for (std::size_t j = 0; j < state.dataSize->FinalSpaceSizing.size(); ++j) {
+                    updateZoneSizingEndZoneSizingCalc5(state.dataSize->FinalSpaceSizing[j], state.dataSize->CalcFinalSpaceSizing[j]);


[src/EnergyPlus/ZoneEquipmentManager.cc:3324]:(style),[shadowVariable],Local variable 'i' shadows outer variable
[src/EnergyPlus/ZoneEquipmentManager.cc:3333]:(style),[shadowVariable],Local variable 'i' shadows outer variable

rraustad · 2024-09-03T14:07:52Z

src/EnergyPlus/ZoneEquipmentManager.cc

-            const Real64 heatLoadRatio = thisZEqList.SequentialHeatingFraction(state, equipNum);
-            const Real64 coolLoadRatio = thisZEqList.SequentialCoolingFraction(state, equipNum);
+            heatLoadRatio = thisZEqList.SequentialHeatingFraction(state, equipNum);
+            coolLoadRatio = thisZEqList.SequentialCoolingFraction(state, equipNum);


[src/EnergyPlus/ZoneEquipmentManager.cc:4272]:(style),[shadowVariable],Local variable 'heatLoadRatio' shadows outer variable
[src/EnergyPlus/ZoneEquipmentManager.cc:4273]:(style),[shadowVariable],Local variable 'coolLoadRatio' shadows outer variable

Myoldmopar · 2024-09-10T13:52:38Z

The conflicts here are ever so slightly more complex than I want to deal with right now. If I need to do it later, I can, but I'll want to grok things more deeply than I would like this morning.

rraustad · 2024-09-10T13:58:49Z

These are a little tricky. I started last night just accepting the develop version and running CppCheck again to see what style suggestions remain. Obviously that did not go well with WindowManager. So don't waste anymore time on these. I'll either fix them or punt and do them over.

…Check-ZoneEquipmentManager

Myoldmopar · 2024-09-11T16:05:32Z

Thanks for resolving things @rraustad. This looks good up here. I'll pull and test.

Myoldmopar · 2024-09-11T16:11:03Z

Still clean with develop, thanks @rraustad

rraustad added 7 commits August 30, 2024 23:50

reduce logic

91f61ab

reduce

e698211

shadow

8df17fb

shadow

237cec8

const

0b176c9

unused and const

eef3254

suspect logic on flag use

6e62170

rraustad added the DoNotPublish Includes changes that shouldn't be reported in the changelog label Aug 31, 2024

rraustad commented Aug 31, 2024

View reviewed changes

rraustad commented Sep 3, 2024

View reviewed changes

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into Cpp…

e2028ee

…Check-ZoneEquipmentManager

Myoldmopar merged commit 84646a7 into develop Sep 11, 2024
10 checks passed

Myoldmopar deleted the CppCheck-ZoneEquipmentManager branch September 11, 2024 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CppCheck ZoneEquipmentManager #10705

CppCheck ZoneEquipmentManager #10705

rraustad commented Aug 31, 2024

rraustad Aug 31, 2024

mjwitte Sep 3, 2024

rraustad Sep 3, 2024

rraustad Sep 3, 2024

rraustad Aug 31, 2024

mjwitte Sep 3, 2024

rraustad Aug 31, 2024

rraustad Aug 31, 2024

Myoldmopar Sep 10, 2024

mjwitte Sep 10, 2024

rraustad Aug 31, 2024

Myoldmopar Sep 10, 2024

mjwitte Sep 10, 2024

rraustad Sep 3, 2024

rraustad Sep 3, 2024

Myoldmopar commented Sep 10, 2024

rraustad commented Sep 10, 2024

Myoldmopar commented Sep 11, 2024

Myoldmopar commented Sep 11, 2024

CppCheck ZoneEquipmentManager #10705

CppCheck ZoneEquipmentManager #10705

Conversation

rraustad commented Aug 31, 2024

Pull request overview

Pull Request Author

Reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Sep 10, 2024

rraustad commented Sep 10, 2024

Myoldmopar commented Sep 11, 2024

Myoldmopar commented Sep 11, 2024