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

CppCheck ZoneContaminantPredictorCorrector #10706

Merged
merged 9 commits into from
Sep 11, 2024

Conversation

rraustad
Copy link
Contributor

Pull request overview

  • Fixes programming style
  • 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.

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

@rraustad rraustad added the DoNotPublish Includes changes that shouldn't be reported in the changelog label Aug 31, 2024
@@ -1121,7 +1118,7 @@ namespace ZoneDehumidifier {
// Calculate and report condensation rate (how much water extracted from the air stream)
// Volumetric flow of water in m3/s for water system interactions

AirInletNodeNum = state.dataZoneDehumidifier->ZoneDehumid(DehumidNum).AirInletNodeNum;
int AirInletNodeNum = state.dataZoneDehumidifier->ZoneDehumid(DehumidNum).AirInletNodeNum;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[src/EnergyPlus/ZoneDehumidifier.cc:1108]:(style),[variableScope],The scope of the variable 'AirInletNodeNum' can be reduced.
[src/EnergyPlus/ZoneDehumidifier.cc:766]:(style),[unreadVariable],Variable 'AirOutletNodeNum' is assigned a value that is never used.

Real64 const CO2GainExceptPeople, // ZOne total CO2 gain from sources except for people
Real64 const ZoneMassFlowRate, // Zone air mass flow rate
Real64 const CO2MassFlowRate, // Zone air CO2 mass flow rate
Real64 const RhoAir // Air density
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[src/EnergyPlus/ZoneContaminantPredictorCorrector.cc:2338]:(style),[constParameter],Parameter 'CO2Gain' can be declared as reference to const
[src/EnergyPlus/ZoneContaminantPredictorCorrector.cc:2339]:(style),[constParameter],Parameter 'CO2GainExceptPeople' can be declared as reference to const
[src/EnergyPlus/ZoneContaminantPredictorCorrector.cc:2340]:(style),[constParameter],Parameter 'ZoneMassFlowRate' can be declared as reference to const
[src/EnergyPlus/ZoneContaminantPredictorCorrector.cc:2341]:(style),[constParameter],Parameter 'CO2MassFlowRate' can be declared as reference to const
[src/EnergyPlus/ZoneContaminantPredictorCorrector.cc:2342]:(style),[constParameter],Parameter 'RhoAir' can be declared as reference to const

int Sch = ScheduleManager::GetCurrentScheduleValue(state, con.GCEmiRateSchedPtr);
if (Sch == 0.0 || state.dataGlobal->BeginEnvrnFlag || state.dataGlobal->WarmupFlag) {
int intSch = ScheduleManager::GetCurrentScheduleValue(state, con.GCEmiRateSchedPtr);
if (intSch == 0.0 || state.dataGlobal->BeginEnvrnFlag || state.dataGlobal->WarmupFlag) {
Copy link
Contributor Author

@rraustad rraustad Sep 1, 2024

Choose a reason for hiding this comment

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

So here, Sch shadows an outer REAL variable. So what was the intent? I think it was to convert a Real64 0 - 1 to either 0 or 1 given the text in the idd. Instead of changing this to read the schedule into a real I chose to int that data just like it was done before. It does seem like this was the intent, except that the idd says if it's 1 it will be set to 0 and here if it's 0 it's set to 0.

[src/EnergyPlus/ZoneContaminantPredictorCorrector.cc:1820]:(style),[shadowVariable],Local variable 'Sch' shadows outer variable

ZoneContaminantSourceAndSink:Generic:DecaySource,
  A3 , \field Schedule Name
       \required-field
       \type object-list
       \object-list ScheduleNames
       \note Value in this schedule should be a fraction (generally 0.0 - 1.0) applied to the
       \note Initial Emission Rate. When the value is equal to 1.0, the time will be reset to
       \note zero.

@@ -388,7 +388,7 @@ namespace Window {
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have two large PRs that should go in this week or next (FLW) and that will have a lot of conflicts with these changes. You may want to hold off on these until those are in.

@Myoldmopar
Copy link
Member

I'll take a pass over resolving the conflicts on the remaining cppcheck PRs, starting with this one. If they end up getting too complex, I'm moving to other PRs.

@Myoldmopar
Copy link
Member

All happy here, merging.

@Myoldmopar Myoldmopar merged commit 4e8c572 into develop Sep 11, 2024
10 checks passed
@Myoldmopar Myoldmopar deleted the CppCheck-ZoneContaminantPredictorCorrector branch September 11, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DoNotPublish Includes changes that shouldn't be reported in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants