Cleanup Cppcheck warnings continued by dareumnam · Pull Request #11514 · NatLabRockies/EnergyPlus

dareumnam · 2026-04-10T13:52:06Z

Pull request overview

Cleanup Cppcheck warnings regarding constVariableReference, knownConditionTrueFalse, and multiCondition

Description of the purpose of this PR

Pull Request Author

Reviewer

…on warnings

…into cppcheck_continued

dareumnam · 2026-04-10T14:04:08Z

            int numNoDepend = -1;
            int loopCount = 0;
-            while ((numNoDepend != 0) || (loopCount > 100000)) {
+            while ((numNoDepend != 0) && (loopCount < 100000)) {


I think this loop condition is flipped.
With while ((numNoDepend != 0) || (loopCount > 100000)), the loop would continue once loopCount exceeds the limit, so the post-loop check if (loopCount > 100000) (line 2411) becomes unreachable.
This probably wants && with a max-iteration guard instead, like while ((numNoDepend != 0) && (loopCount < 100000)).

dareumnam · 2026-04-10T14:05:43Z

    const auto &FinalSysSizing(state.dataSize->FinalSysSizing);
-    auto &EvapCond(state.dataEvapCoolers->EvapCond);
-    auto &thisEvapCond(EvapCond(EvapCoolNum));
+    auto &thisEvapCond(state.dataEvapCoolers->EvapCond(EvapCoolNum));


EvapCond itself is only used once and is never modified, and the actual mutable object is thisEvapCond. Rather than making EvapCond a const, it may be cleaner to remove that alias entirely and bind thisEvapCond directly to state.dataEvapCoolers->EvapCond(EvapCoolNum).

dareumnam · 2026-04-10T14:07:26Z

-        if (!RunFlag) {
-            return;
-        }
-


The second if (!RunFlag) here is unreachable since the earlier if (!RunFlag) in line 1590 already returns from the function. By the time execution gets here, RunFlag must be true, so this check can be removed.

dareumnam · 2026-04-10T14:09:00Z

            }
            if (state.dataEnvrn->PrintEnvrnStampWarmup) {
-                if (state.dataReportFlag->PrintEndDataDictionary && state.dataGlobal->DoOutputReporting) {
+                if (state.dataReportFlag->PrintEndDataDictionary) {


DoOutputReporting is already guaranteed to be true by the enclosing else if condition (line 3356), so the inner check is redundant here. I removed the repeated state.dataGlobal->DoOutputReporting from the nested if condition.

dareumnam · 2026-04-10T14:10:17Z

                    print(state.files.mtr, "{}\n", EndOfHeaderString);
                    state.dataReportFlag->PrintEndDataDictionary = false;
                }
-                if (state.dataGlobal->DoOutputReporting) {


Same, the repeated state.dataGlobal->DoOutputReporting can be removed.

dareumnam · 2026-04-10T14:12:25Z

            }
            if (state.dataEnvrn->PrintEnvrnStampWarmup) {
-                if (state.dataReportFlag->PrintEndDataDictionary && state.dataGlobal->DoOutputReporting && !state.dataHVACMgr->PrintedWarmup) {
+                if (state.dataReportFlag->PrintEndDataDictionary && !state.dataHVACMgr->PrintedWarmup) {


Same as the HeatBalanceManager.cc cases. DoOutputReporting is already guaranteed to be true by the enclosing else if condition (line 479), so the inner checks are redundant here and in the line 493. I removed the repeated state.dataGlobal->DoOutputReporting from the nested if condition.

dareumnam · 2026-04-10T14:15:59Z

                            PreviousMaxiumHumidOrDehumidOutput = latentRoomORZone;
                        }
                    } else {
-                        if (!DidWeMeetLoad) {


!DidWeMeetLoad is already guaranteed by the enclosing if (!DidWeMeetLoad && !DidWeMeetHumidificaiton) in line 1544, so the nested if (!DidWeMeetLoad) is redundant here and can be removed.

dareumnam · 2026-04-10T14:17:08Z

            CurrentOperatingSettings[1] = oStandBy;
        } else {
            // if we partly met the load then do the best we can and run full out in that optimal setting.
-            if (!DidWeMeetLoad && DidWePartlyMeetLoad) {


dareumnam · 2026-04-10T14:21:43Z

        if (!RunFlag) {
            DesiredMassFlowRate = 0.0;

-        } else if (RunFlag && this->InternalFlowControl) {


RunFlag is already implied in these else if branches because the preceding if (!RunFlag) (line 1191) has already failed. The four conditions here can be simplified by removing RunFlag &&.

dareumnam · 2026-04-10T14:22:35Z

-
-        if (ErrorsFound) {
-            ShowFatalError(state, "Preceding sizing errors cause program termination");
-        }


ErrorsFound is initialized to false and never updated in this routine, so the final if (ErrorsFound) block is unreachable.

dareumnam · 2026-04-10T14:26:04Z

    int SurfDetails = 0;
    bool SurfVert = false;
    bool SurfDet = false;
    bool DXFDone = false;


These three are initialized to false.

dareumnam · 2026-04-10T14:26:36Z


    General::ScanForReports(state, "Surfaces", DoReport, "Vertices");
    if (DoReport) {
-        if (!SurfVert) {


These flags here and below are being checked in places where they are still guaranteed to be false, so the nested if (!flag) branches are redundant.

dareumnam · 2026-04-10T14:27:49Z

    General::ScanForReports(state, "Surfaces", DoReport, "VRML", Option1, Option2);
    if (DoReport) {
-        bool VRMLDone = false;
-        if (!VRMLDone) {


VRMLDone is declared locally inside the block and never set to true, so its else branch below is unreachable. the if-else condition can be removed.

dareumnam · 2026-04-10T14:30:49Z

@@ -3464,7 +3464,7 @@ namespace PlantPipingSystemsManager {
                            if (CellXIndex <= MinXIndex || CellZIndex <= MinZIndex) { // Ground surface


If CellXIndex <= MinXIndex || CellZIndex <= MinZIndex is false, then both indices must already be greater than the minimum bounds, so the following else if (CellXIndex >= MinXIndex || CellZIndex >= MinZIndex) (line 3467) is always true. This can be simplified to a plain else.

dareumnam · 2026-04-10T14:32:32Z

-        auto &AirChillerSet = state.dataRefrigCase->AirChillerSet;
        for (int CoilSetIndex = 1; CoilSetIndex <= state.dataRefrigCase->NumRefrigChillerSets; ++CoilSetIndex) {
-            AirChillerSet(CoilSetIndex).CalculateAirChillerSets(state);
+            state.dataRefrigCase->AirChillerSet(CoilSetIndex).CalculateAirChillerSets(state);


I just removed the one-use AirChillerSet alias and called it directly. That gets rid of the constVariableReference warning and makes it a bit clearer that the element itself is being mutated.

dareumnam · 2026-04-10T14:34:00Z


    // Sum up all the case and walk-in loads served by the secondary loop
    if (this->NumCases > 0) {
-        auto &RefrigCase = state.dataRefrigCase->RefrigCase;


Removed the one-use RefrigCase alias and accessed the element directly. This clears the constVariableReference warning and keeps the mutating call explicit.

dareumnam · 2026-04-10T14:34:48Z

        } // CaseNum
    } // NumCases > 0
    if (this->NumWalkIns > 0) {
-        auto &WalkIn = state.dataRefrigCase->WalkIn;


Removed the one-use WalkIn alias and accessed the element directly. This clears the constVariableReference warning and keeps the mutating call explicit.

dareumnam · 2026-04-10T14:35:23Z

        }

        if (state.dataRefrigCase->NumSimulationWalkIns > 0) {
-            auto &WalkIn = state.dataRefrigCase->WalkIn;


Removed the one-use WalkIn alias and accessed the element directly. This clears the constVariableReference warning and keeps the mutating call explicit.

dareumnam · 2026-04-10T14:37:21Z


    // NOTE: Performed before checking min and max constraints to mimic original implementation
    //       in ManagerControllers()
    if (RootFinderData.MinPoint.DefinedFlag && RootFinderData.MaxPoint.DefinedFlag) {


RootFinderData.MinPoint.DefinedFlag is already guaranteed by the enclosing condition here,

dareumnam · 2026-04-10T14:37:30Z

-            if (CheckMinConstraint(state, RootFinderData)) {
-                RootFinderData.StatusFlag = RootFinderStatus::OKMin;
-                RootFinderData.XCandidate = RootFinderData.MinPoint.X;
+        if (CheckMinConstraint(state, RootFinderData)) {


so this nested check is redundant and can be removed.

dareumnam · 2026-04-10T14:38:35Z


        } else if (is_any_of(NextChar, "+-*/^=<>@|&")) {
            // Parse an operator token
            if (NextChar == '-') {


(NextChar == '-') is already guaranteed here

dareumnam · 2026-04-10T14:39:49Z

                        }
                    }

                    if ((NumOperands == 5) && (NumTokens >= 6)) {


(NumOperands == 5) is already guaranteed here.

dareumnam · 2026-04-10T14:40:05Z

                    ++NumErrors;
                    DivFound = false;
-                } else if (OperatorProcessing && (NextChar == '-')) {
+                } else if (OperatorProcessing) {


so this nested check is redundant and can be removed.

dareumnam · 2026-04-10T14:40:17Z

                        state.dataRuntimeLang->ErlExpression(ExpressionNum).Operand(5).Number = Token(Pos + 5).Number;
                        state.dataRuntimeLang->ErlExpression(ExpressionNum).Operand(5).Expression = Token(Pos + 5).Expression;
                        state.dataRuntimeLang->ErlExpression(ExpressionNum).Operand(5).Variable = Token(Pos + 5).Variable;
-                        if ((NumOperands == 5) && (NumTokens - 6 > 0)) { // too many tokens for this non-binary operator


so this nested check is redundant and can be removed.

dareumnam · 2026-04-10T15:03:42Z

            // Need to make sure that flows are greater than zero
            if (MassFlowSet >= 0.0) {
                state.dataLoopNodes->Node(NodeNumOut).MassFlowRateSetPoint = MassFlowSet;
-            } else if (MassFlowSet < 0.0) {


This is redundant since it is the exact opposite of the preceding if (MassFlowSet >= 0.0) in two lines above.

dareumnam · 2026-04-10T21:12:08Z

-
-    if (ErrorsFound) {
-        ShowFatalError(state, EnergyPlus::format("{}Errors found in input.  Preceding condition(s) cause termination.", RoutineName));
-    }


ErrorsFound is initialized to false and never updated in this routine, so the final fatal check is unreachable.

dareumnam · 2026-04-10T21:13:24Z

-                            OASysEqSizing(state.dataSize->CurOASysNum).AirFlow = true;
-                            OASysEqSizing(state.dataSize->CurOASysNum).AirVolFlow = finalSysSizing.DesOutAirVolFlow;
+                            state.dataSize->OASysEqSizing(state.dataSize->CurOASysNum).AirFlow = true;
+                            state.dataSize->OASysEqSizing(state.dataSize->CurOASysNum).AirVolFlow = finalSysSizing.DesOutAirVolFlow;


Removed the one-use OASysEqSizing alias and used the element directly. This clears the constVariableReference warning and keeps the mutating call explicit.

dareumnam · 2026-04-10T21:15:35Z

                      TempAbsPlate * (hConvCoefA2C + hRadCoefA2C);
            tempdenom = (hConvCoefC2O + hRadCoefC2O) + (hConvCoefA2C + hRadCoefA2C);
            TempOuterCover = tempnom / tempdenom;
        } else if (NumCovers == 2) {


In this NumCovers == 2 branch, the loop index Num can only be 0 or 1. That means once if (Num == 0) is false, Num == 1 is guaranteed, so the else if (Num == 1) can be simplified to a plain else.

dareumnam · 2026-04-10T21:17:20Z

                A1eqout = Atop + 0.5 * Abot * (Al + Ar + Ah) / (Abot + Atop);
                A2eqin = Atop + 0.5 * Abot * (Al + Ar + Ah) / (Abot + Atop);
-            } else if (Tgap1 < Tgap2) {
+            } else {


The else if (Tgap1 < Tgap2) is redundant since it is the exact complement of the preceding if (Tgap1 >= Tgap2). This can be simplified to a plain else for the same intended behavior.

dareumnam · 2026-04-10T21:30:10Z

                Tup = (alpha1 * Tav1 + beta1 * alpha2 * Tav2) / (1.0 - beta1 * beta2);
                Tdown = alpha2 * Tav2 + beta2 * Tup;
-            } else if (Tgap2 >= Tgap1) {
+            } else {


The else if (Tgap2 >= Tgap1) is redundant since it is the exact complement of the preceding if (Tgap1 > Tgap2). This can be simplified to a plain else for the same intended behavior.

dareumnam · 2026-04-10T21:30:47Z

                Temp1 = Tav1 - (H01 / H) * (Tup - Tdown);
                Temp2 = Tav2 - (H02 / H) * (Tdown - Tup);
-            } else if (Tgap2 >= Tgap1) {
+            } else {


The else if (Tgap2 >= Tgap1) is redundant since it is the exact complement of the preceding if (Tgap1 > Tgap2). This can be simplified to a plain else for the same intended behavior.

dareumnam · 2026-04-10T21:31:16Z

            qv1 = -dens1 * cp1 * speed1 * s1 * L * (Tdown - Tup) / (H * L);
            qv2 = -dens2 * cp2 * speed2 * s2 * L * (Tup - Tdown) / (H * L);
-        } else if (Tgap2 < Tgap1) {
+        } else {


The else if (Tgap2 < Tgap1) is redundant since it is the exact complement of the preceding if (Tgap2 >= Tgap1). This can be simplified to a plain else for the same intended behavior.

dareumnam · 2026-04-10T21:31:46Z

                A1eqin = Abot + 0.5 * Atop * (Al + Ar + Ah) / (Abot + Atop);
                A1eqout = Atop + 0.5 * Abot * (Al + Ar + Ah) / (Abot + Atop);
-            } else if (Tgap <= Tenv) {
+            } else {


The else if (Tgap <= Tenv) is redundant since it is the exact complement of the preceding if (Tgap > Tenv). This can be simplified to a plain else for the same intended behavior.

dareumnam · 2026-04-10T21:32:18Z

-                ZoneEqSizing(state.dataSize->CurZoneEqNum).CoolingAirVolFlow = state.dataSize->DataNonZoneNonAirloopValue;
+                state.dataSize->ZoneEqSizing(state.dataSize->CurZoneEqNum).CoolingAirFlow = true;
+                state.dataSize->ZoneEqSizing(state.dataSize->CurZoneEqNum).CoolingAirVolFlow = state.dataSize->DataNonZoneNonAirloopValue;
            }


Removed the one-use ZoneEqSizing alias and used the element directly. This clears the constVariableReference warning and keeps the mutating call explicit.

@dareumnam here's my thoughts on this.

Never use a class reference:

auto &ZoneEqSizing = state.dataSize->ZoneEqSizing;

and instead use an object reference:

auto &ZoneEqSizing = state.dataSize->ZoneEqSizing(state.dataSize->CurZoneEqNum);

but don't create the reference unless there are more than 2 usages (that's just my personal preference).

So in this case I agree with this change. You could have used the object reference here and that would still be OK because it was used twice. Creating a reference to use it one time just adds more code than is needed.

Your code change here seems fine to me.

dareumnam · 2026-04-10T21:34:22Z

-                LOK = true;
-                DOK = true;
-                BOK = true;
            }


LOK, DOK, and BOK are assigned throughout this routine but never actually checked afterward, so cppcheck looks right here. It seems these calls are being made for their side effects on L.LWP_EL / L.SWP_EL, while the bool return values are currently unused. If the return status is not needed, the locals can probably be removed. Otherwise this may be missing an error check.

I don't want to add to the effort here but if these locals are not needed then do these 12 functions even need to return a bool? e.g., bool PD_LWP{ .. return PD_LWP}; Maybe the developer used these bools for debugging and never removed them?

dareumnam added 2 commits April 10, 2026 02:20

fix knownConditionTrueFalse, ConstVariableReference, and multiConditi…

94b80a9

…on warnings

Merge branch 'develop' of https://github.com/NatLabRockies/EnergyPlus …

bf1e6f9

…into cppcheck_continued

dareumnam added the Defect Includes code to repair a defect in EnergyPlus label Apr 10, 2026

dareumnam changed the title ~~Fix a logic/scoping issue in SimAirServingZones.cc~~ Cleanup CppCheck warnings continued Apr 10, 2026

dareumnam changed the title ~~Cleanup CppCheck warnings continued~~ Cleanup Cppcheck warnings continued Apr 10, 2026

dareumnam marked this pull request as draft April 10, 2026 13:56

dareumnam added DoNotPublish Includes changes that shouldn't be reported in the changelog and removed Defect Includes code to repair a defect in EnergyPlus labels Apr 10, 2026

dareumnam commented Apr 10, 2026

View reviewed changes

mitchute added 2 commits April 10, 2026 14:18

cleanup unused var

cb75cae

Merge branch 'develop' into cppcheck_continued

6196d59

dareumnam commented Apr 10, 2026

View reviewed changes

		@@ -3464,7 +3464,7 @@ namespace PlantPipingSystemsManager {
		if (CellXIndex <= MinXIndex \|\| CellZIndex <= MinZIndex) { // Ground surface

Conversation

dareumnam commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request overview

Description of the purpose of this PR

Pull Request Author

Reviewer

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dareumnam Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dareumnam Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dareumnam Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dareumnam Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dareumnam commented Apr 10, 2026 •

edited

Loading

dareumnam Apr 10, 2026 •

edited

Loading

dareumnam Apr 10, 2026 •

edited

Loading

dareumnam Apr 10, 2026 •

edited

Loading

dareumnam Apr 10, 2026 •

edited

Loading