CppCheck FluidProperties through HVACUnitaryBypassVAV #10668

rraustad · 2024-08-19T23:55:28Z

Pull request overview

Fixes programming issues
Uses CppCheck as a 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.

amirroth · 2024-08-19T23:58:33Z

src/EnergyPlus/FluidProperties.cc

@@ -2039,9 +2037,8 @@ namespace FluidProperties {
        // for the refrigerant properties.
        // Most properties requested (e.g., Specific Heat) must be > 0 but the tables may
        // be set up for symmetry and not be limited to just valid values.
-        auto &df = state.dataFluidProps;


I thought we were not supposed to reference a class? Maybe this is different. I can easily put these back.

A class/struct is about the only thing you should reference, what you shouldn't reference is scalars and arrays.

amirroth · 2024-08-19T23:58:58Z

src/EnergyPlus/FluidProperties.cc

@@ -2759,8 +2752,6 @@ namespace FluidProperties {

        int LoTempIndex = FindArrayIndex(Temperature, this->PsTemps, this->PsLowTempIndex, this->PsHighTempIndex);

-        auto &df = state.dataFluidProps;


Why undo this?

If I'm seeing it right, it looks like this variable just got moved into a smaller scope, not undoing the reference usage.

The references are staying in place (I put them all back in) and only moving if there was a CppCheck reduce scope suggestion. I do want to see CI results though because the Mac results are troubling.

Just to be clear, Mac results are not reliable right now. There is something very odd going on that I'm trying to investigate. Gort and Tik-Tok are doing fine though.

rraustad

What I see in CppCheck

rraustad · 2024-08-19T23:56:46Z

src/EnergyPlus/FluidProperties.cc

@@ -1035,7 +1035,7 @@ namespace FluidProperties {
                continue;
            }

-            auto &supTempArray = FluidTemps(supTempArrayNum);
+            auto const &supTempArray = FluidTemps(supTempArrayNum);


[src/EnergyPlus/FluidProperties.cc:1038]:(style),[constVariable],Variable 'supTempArray' can be declared as reference to const

rraustad · 2024-08-19T23:57:30Z

src/EnergyPlus/FluidProperties.cc

@@ -1441,7 +1441,7 @@ namespace FluidProperties {

            if (!glycolRaw->CpTempArrayName.empty()) {
                int cpTempArrayNum = Util::FindItemInList(glycolRaw->CpTempArrayName, FluidTemps);
-                auto &cpTempArray = FluidTemps(cpTempArrayNum);
+                auto const &cpTempArray = FluidTemps(cpTempArrayNum);


[src/EnergyPlus/FluidProperties.cc:1444]:(style),[constVariable],Variable 'cpTempArray' can be declared as reference to const

rraustad · 2024-08-19T23:57:51Z

src/EnergyPlus/FluidProperties.cc

@@ -1452,7 +1452,7 @@ namespace FluidProperties {

            if (!glycolRaw->RhoTempArrayName.empty()) {
                int rhoTempArrayNum = Util::FindItemInList(glycolRaw->RhoTempArrayName, FluidTemps);
-                auto &rhoTempArray = FluidTemps(rhoTempArrayNum);
+                auto const &rhoTempArray = FluidTemps(rhoTempArrayNum);


[src/EnergyPlus/FluidProperties.cc:1455]:(style),[constVariable],Variable 'rhoTempArray' can be declared as reference to const

rraustad · 2024-08-19T23:58:02Z

src/EnergyPlus/FluidProperties.cc

@@ -1463,7 +1463,7 @@ namespace FluidProperties {

            if (!glycolRaw->CondTempArrayName.empty()) {
                int condTempArrayNum = Util::FindItemInList(glycolRaw->CondTempArrayName, FluidTemps);
-                auto &condTempArray = FluidTemps(condTempArrayNum);
+                auto const &condTempArray = FluidTemps(condTempArrayNum);


[src/EnergyPlus/FluidProperties.cc:1466]:(style),[constVariable],Variable 'condTempArray' can be declared as reference to const

rraustad · 2024-08-19T23:58:11Z

src/EnergyPlus/FluidProperties.cc

@@ -1474,7 +1474,7 @@ namespace FluidProperties {

            if (!glycolRaw->ViscTempArrayName.empty()) {
                int viscTempArrayNum = Util::FindItemInList(glycolRaw->ViscTempArrayName, FluidTemps);
-                auto &viscTempArray = FluidTemps(viscTempArrayNum);
+                auto const &viscTempArray = FluidTemps(viscTempArrayNum);


[src/EnergyPlus/FluidProperties.cc:1477]:(style),[constVariable],Variable 'viscTempArray' can be declared as reference to const

rraustad · 2024-08-20T00:10:46Z

src/EnergyPlus/HVACStandAloneERV.cc

@@ -297,7 +296,6 @@ void GetStandAloneERV(EnergyPlusData &state)
        standAloneERV.ExhaustAirFanName = Alphas(5);
        GlobalNames::IntraObjUniquenessCheck(
            state, Alphas(5), CurrentModuleObject, cAlphaFields(5), state.dataHVACStandAloneERV->ExhaustAirFanUniqueNames, ErrorsFound);
-        errFlag = false;


[src/EnergyPlus/HVACStandAloneERV.cc:300]:(style),[redundantAssignment],Variable 'errFlag' is reassigned a value before the old one has been used.

rraustad · 2024-08-20T00:11:09Z

src/EnergyPlus/HVACStandAloneERV.cc

@@ -1695,7 +1693,6 @@ int getEqIndex(EnergyPlusData &state, std::string_view CompName)
    for (int StandAloneERVNum = 1; StandAloneERVNum <= state.dataHVACStandAloneERV->NumStandAloneERVs; StandAloneERVNum++) {
        if (Util::SameString(CompName, state.dataHVACStandAloneERV->StandAloneERV(StandAloneERVNum).Name)) {
            return StandAloneERVNum;
-            break;


[src/EnergyPlus/HVACStandAloneERV.cc:1698]:(style),[duplicateBreak],Consecutive return, break, continue, goto or throw statements are unnecessary.

rraustad · 2024-08-20T00:11:37Z

src/EnergyPlus/HVACUnitaryBypassVAV.cc

@@ -330,7 +330,6 @@ namespace HVACUnitaryBypassVAV {
        bool ErrorsFound(false);      // Set to true if errors in input, fatal at end of routine
        bool DXErrorsFound(false);    // Set to true if errors in get coil input
        Array1D_int OANodeNums(4);    // Node numbers of OA mixer (OA, EA, RA, MA)
-        std::string HXDXCoolCoilName; // Name of DX cooling coil used with Heat Exchanger Assisted Cooling Coil


[src/EnergyPlus/HVACUnitaryBypassVAV.cc:333]:(style),[unusedVariable],Unused variable: HXDXCoolCoilName

rraustad · 2024-08-20T00:12:07Z

src/EnergyPlus/HVACUnitaryBypassVAV.cc

@@ -754,7 +753,7 @@ namespace HVACUnitaryBypassVAV {
                            ShowContinueError(state, format("...occurs in {} \"{}\"", thisCBVAV.UnitType, thisCBVAV.Name));
                            ErrorsFound = true;
                        } else {
-                            auto &newCoil = state.dataCoilCooingDX->coilCoolingDXs[thisCBVAV.DXCoolCoilIndexNum];
+                            auto const &newCoil = state.dataCoilCooingDX->coilCoolingDXs[thisCBVAV.DXCoolCoilIndexNum];


[src/EnergyPlus/HVACUnitaryBypassVAV.cc:757]:(style),[constVariable],Variable 'newCoil' can be declared as reference to const

rraustad · 2024-08-20T00:12:42Z

src/EnergyPlus/HVACUnitaryBypassVAV.cc

@@ -2444,7 +2443,7 @@ namespace HVACUnitaryBypassVAV {
                                }
                                // now find the speed ratio for the found speednum
                                auto f = [&state, CBVAVNum, SpeedNum, DesOutTemp](Real64 const SpeedRatio) {
-                                    auto &thisCBVAV = state.dataHVACUnitaryBypassVAV->CBVAV(CBVAVNum);
+                                    auto const &thisCBVAV = state.dataHVACUnitaryBypassVAV->CBVAV(CBVAVNum);


[src/EnergyPlus/HVACUnitaryBypassVAV.cc:2447]:(style),[constVariable],Variable 'thisCBVAV' can be declared as reference to const

Myoldmopar

These look like nice changes. Lots of additional const attributes and lots of reduced scope. 👍 Are you planning to go further on this PR? If not, it should be able to slide in without any major issues.

Myoldmopar · 2024-08-20T14:36:27Z

src/EnergyPlus/FluidProperties.cc

@@ -2759,8 +2752,6 @@ namespace FluidProperties {

        int LoTempIndex = FindArrayIndex(Temperature, this->PsTemps, this->PsLowTempIndex, this->PsHighTempIndex);

-        auto &df = state.dataFluidProps;


If I'm seeing it right, it looks like this variable just got moved into a smaller scope, not undoing the reference usage.

rraustad · 2024-08-20T16:52:32Z

@Myoldmopar I am not making any other changes on this branch.

Myoldmopar · 2024-08-20T18:17:41Z

Looks like all the reasonable CI machines are reporting happily, and this removes 26 or so messages from the cppcheck results, cleaning up the code nicely without affecting functionality. Good stuff here, and I'm not waiting to merge. Thanks @rraustad

FluidProperties through HVACUnitaryBypassVAV

842b109

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

rraustad added this to the EnergyPlus 24.2 milestone Aug 19, 2024

amirroth reviewed Aug 19, 2024

View reviewed changes

rraustad commented Aug 20, 2024

View reviewed changes

rraustad added 2 commits August 20, 2024 08:26

Revert shortcut removal

a0f066f

More reverts

266dfa2

Myoldmopar reviewed Aug 20, 2024

View reviewed changes

Myoldmopar merged commit f274ccb into develop Aug 20, 2024
14 of 15 checks passed

Myoldmopar deleted the CppCheck-FluidProperties branch August 20, 2024 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CppCheck FluidProperties through HVACUnitaryBypassVAV #10668

CppCheck FluidProperties through HVACUnitaryBypassVAV #10668

rraustad commented Aug 19, 2024

amirroth Aug 19, 2024

rraustad Aug 20, 2024

amirroth Aug 20, 2024 •

edited

Loading

amirroth Aug 19, 2024

Myoldmopar Aug 20, 2024

rraustad Aug 20, 2024

Myoldmopar Aug 20, 2024

rraustad left a comment

rraustad Aug 19, 2024

rraustad Aug 19, 2024

rraustad Aug 19, 2024

rraustad Aug 19, 2024

rraustad Aug 19, 2024

rraustad Aug 20, 2024

rraustad Aug 20, 2024

rraustad Aug 20, 2024

rraustad Aug 20, 2024

rraustad Aug 20, 2024

Myoldmopar left a comment

Myoldmopar Aug 20, 2024

rraustad commented Aug 20, 2024

Myoldmopar commented Aug 20, 2024

		@@ -2759,8 +2752,6 @@ namespace FluidProperties {

		int LoTempIndex = FindArrayIndex(Temperature, this->PsTemps, this->PsLowTempIndex, this->PsHighTempIndex);

		auto &df = state.dataFluidProps;

CppCheck FluidProperties through HVACUnitaryBypassVAV #10668

CppCheck FluidProperties through HVACUnitaryBypassVAV #10668

Conversation

rraustad commented Aug 19, 2024

Pull request overview

Pull Request Author

Reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirroth Aug 20, 2024 • edited Loading

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

rraustad left a 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rraustad commented Aug 20, 2024

Myoldmopar commented Aug 20, 2024

amirroth Aug 20, 2024 •

edited

Loading