Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert const int/double to constexpr Where Possible #9130

Merged
merged 8 commits into from
Nov 2, 2021

Conversation

mitchute
Copy link
Collaborator

Pull request overview

  • Converts const int and double to constexpr where possible.
  • I also note a couple of locations where const vars exist in state. These should eventually be removed.

@mitchute mitchute self-assigned this Oct 13, 2021
@mitchute mitchute added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Oct 13, 2021
@mitchute mitchute added this to the EnergyPlus 2022.1 milestone Oct 13, 2021
Real64 constexpr OutdoorUnitInletAirDryBulbTemp(27.78); // 27.78C (82F) Test B (for SEER)
Real64 constexpr OutdoorUnitInletAirDryBulbTempRated(35.0); // 35.00C (95F) Test A (rated capacity)
Real64 constexpr AirMassFlowRatioRated(1.0); // AHRI test is at the design flow rate so AirMassFlowRatio is 1.0
Real64 constexpr PLRforSEER(0.5); // Part-load ratio for SEER calculation (single speed DX cooling coils)
Array1D<Real64> const ReducedPLR(4, {1.0, 0.75, 0.50, 0.25}); // Reduced Capacity part-load conditions
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These cats can be turned into constexpr std::array<Real64>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll work on replacing these, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll work on replacing these, too.

Ha. Haha. OK, that was naive of me. These aren't going to happen in this PR. I'll work on these separately.

@@ -1263,7 +1263,7 @@ namespace CurveManager {
state.dataCurveManager->PerfCurve(CurveNum).CurveMaxPresent = true;
}

const int NumVar = 5;
constexpr int NumVar = 5;
std::string VarNames[NumVar] = {"v", "w", "x", "y", "z"};
Copy link
Collaborator

@amirroth amirroth Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind C-arrays, but I think constexpr std::array is preferred. Also, std::string could be std::string_view.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll probably pass on the std::string to std::string_view for this round. I haven't spent much time working with string_view, but IIRC they probably are going to require some other changes at the use point.

@@ -104,6 +104,7 @@ struct SystemVarsData : BaseGlobalStruct
{
bool firstTime = true;

// TODO: remove 'const' from state
int const iASCII_CR = 13; // endline value when just CR instead of CR/LF
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constexpr?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's inside of a state subclass. These (and others) need to be pulled out at some point.

@@ -1737,7 +1737,7 @@ void GshpSpecs::CalcWatertoWaterHPCooling(EnergyPlusData &state, Real64 const My

// SUBROUTINE PARAMETER DEFINITIONS:
Real64 const CelsiustoKelvin(DataGlobalConstants::KelvinConv); // Conversion from Celsius to Kelvin
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not this one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because my script wasn't smart enough to find it. I'll fix this one up.

@@ -240,7 +240,7 @@ void GetPumpInput(EnergyPlusData &state)
using ScheduleManager::GetScheduleIndex;

// SUBROUTINE PARAMETER DEFINITIONS:
Real64 const StartTemp(100.0); // Standard Temperature across code to calculated Steam density
Real64 constexpr StartTemp(100.0); // Standard Temperature across code to calculated Steam density
static constexpr std::string_view RoutineName("GetPumpInput: ");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Name/NameNoColon pattern is cray-cray.

@@ -425,19 +425,21 @@ int ArgCheck(EnergyPlusData &state,
return ArgCheck;
}

if BITF_TEST_NONE (BITF(LayerType(i)),
if
BITF_TEST_NONE(BITF(LayerType(i)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to put () around these even though the macro itself has ()s inside of it. We should probably turn the BITF stuff from macros to constexpr functions just to enforce this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ❤️ ❤️ the idea of turning the BITF stuff into proper functions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, especially since functions can be made constexpr. I've already put something about this on the wiki

@Myoldmopar
Copy link
Member

This is a great big set of changes, that further emphasize the parts of the code that are known at compile time. This is great. I will pull develop in here, and I realize there are a couple things mentioned above that should be pursued next (string view, some extern thing), but this should be wrapped up. Any further comments before this merges, assuming it comes back clean?

@Myoldmopar
Copy link
Member

All clean, merging, thanks!

@Myoldmopar Myoldmopar merged commit 264b295 into develop Nov 2, 2021
@Myoldmopar Myoldmopar deleted the convert-const-to-constexpr branch November 2, 2021 15:51
yujiex pushed a commit that referenced this pull request Jan 27, 2022
Convert `const` `int`/`double` to `constexpr` Where Possible
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants