#11681 - Enable -Wshadow-field and investigate actual bugs it finds

[jmarrec]

Pull request overview

• Fixes Enable Wshadow-field and investigate actual bugs it finds #11681

• This builds upon Fix #11680 - Completely Remove the use of link_options / add_compile_options in third_party #11683 - review and merge that one first

  • To review right now, exclude commits from that PR: https://github.com/NatLabRockies/EnergyPlus/pull/11684/changes/25e3884443bec67c075ae9c36d2405aba13b31ab..HEAD

Description of the purpose of this PR

Enable -Wshadow-field on clang and fixup the warnings. Found a few fixable bugs.

Found another one that will warrant another issue in HeatPumpAirToWater, for now I restored the behavior to the current so I wouldn't produce diff, but it seems that there are more than a few issues.

Pull Request Author

• 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
• If adding/removing any output files (e.g., eplustbl.*)
  • Update ..\scripts\Epl-run.bat
  • Update ..\scripts\RunEPlus.bat
  • Update ..\src\EPLaunch\ MainModule.bas, epl-ui.frm, and epl.vbp (VersionComments)
  • Update ...github\workflows\energyplus.py

Reviewer

• 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

[jmarrec]

New flag on clang. Like I said, -Wshadow is way too noisy

[jmarrec]

Wshadow revealed a big inheritance mistake in SteamBaseboardRadiator

So there’s a ZoneHVAC:Baseboard:RadiantConvective:Steam and a ZoneHVAC:Baseboard:RadiantConvective:Steam:Design objects

The C++ SteamBaseboardParams instance has an int index to find the SteamBaseboardDesignData...

But the SteamBaseboardDesignData inherits from SteamBaseboardParams, so it carries EVERY field from SteamBaseboardParams, so about 656 bytes for no reason!

It shouldn't inherit at all!

[jmarrec]

Same issue as the Steam counterpart

[jmarrec]

oneTimeInitFlag is defined in PlantComponent, which is inherits, but we need that flag here as it's used to call SetupOutputVariables

[jmarrec]

EIRPlantLoopHeatPump defines int indices

struct HeatPumpAirToWater : public EIRPlantLoopHeatPump would redefine it: it shadows with a different type. Not great.

Ideally we add a different Base but this was too much change for not so much gains, so I just defined a different one.

[jmarrec]

This is a hard bug. Here I'm voluntarily restoring the behavior to avoid a diff.

But there's a problem in there.

Needs further investigation (separate, pre-existing issue, not caused by this branch).

HeatPumpAirToWater::sizeLoadSide() is not virtual, and its only would-be caller (EIRPlantLoopHeatPump::onInitLoopEquip) invokes this->sizeLoadSide(state) from within the base class's own scope, so that call always statically resolves to EIRPlantLoopHeatPump::sizeLoadSide,
never the derived override.
Confirmed no production or test code calls sizeLoadSide() through a HeatPumpAirToWater*-typed pointer either, so HeatPumpAirToWater::sizeLoadSide() (and its referenceCapacityOneUnit
recompute) is effictively dead code today. Left a TODO comment at the definition;

This likely needs a real fix (e.g. making sizeLoadSide virtual, or overriding onInitLoopEquip in HeatPumpAirToWater) but that's a behavior (and diff-producing) change out of scope for this PR.

[jmarrec]

• Filed Inheritance and shadowing issues in HeatPumpAirToWater #11685

[jmarrec]

This code is effectively dead currently, which I suppose isn't the intent given this function does post-calculations...

[jmarrec]

EnergyPlus/src/EnergyPlus/PlantLoopHeatPumpEIR.cc

Line 1275 in 5f5c37c

if (this->companionHeatPumpCoil != nullptr) {

In EIRPlantLoopHeatPump::sizeLoadSide, the companionHeatingCoil was ALWAYS nullptr for HeatPumpAirToWater (and it is still, but it's more explicit now).

HeatPumpAirToWater would initialize its own shadowing copy of companionHeatPumpCoil and leave the base one unitialized.

And it's the Base method that's called so in there companionHeatPumpCoil is definitely the Base's, not the Derived

I have a not so MCVE at https://compiler-explorer.com/z/3vYjEj1oY that shows it, and a more MCVE (less complete) at https://gcc.godbolt.org/z/YrMY9P3q8

[jmarrec]

Removed BaseThermalPropertySet Properties; from BaseCell, and instead gave each of its three derived structs its own appropriately-typed Properties member directly:

• RadialCellInformation → BaseThermalPropertySet Properties; (new, explicit)
• CartesianCell → BaseThermalPropertySet Properties; (new, explicit)
• FluidCellInformation → kept its existing ExtendedFluidProperties Properties; (unchanged)

Why: FluidCellInformation was redeclaring Properties with the wider ExtendedFluidProperties type (adds Viscosity/Prandtl) to shadow the inherited BaseThermalPropertySet Properties from BaseCell — needed so that bulk-assignments like cell.PipeCellData.Fluid.Properties = thisCircuit->CurFluidPropertySet; copy the full extended struct instead of object-slicing it. This triggered a -Wshadow-field warning, and — since C++ field-hiding isn't virtual dispatch — meant every FluidCellInformation instance carried a second, entirely dead BaseThermalPropertySet subobject inherited from BaseCell that was never read or written.

Since BaseCell itself is never instantiated or referenced directly (only used as a base for these three structs), moving Properties down into each derived struct removes the shadowing entirely and eliminates the wasted subobject, with no behavior change — RadialCellInformation and CartesianCell still get the same BaseThermalPropertySet Properties they had before, just declared locally instead of inherited