This looks like an old error of mine but the comment really should say "native variables next"

enum class is a scope. No need to have the same words in both the type name and the type value.

These can be constexpr std::string_views in the .cc file. Don't need to be member string objects.

😊

This three-way option was previously implemented using two bool, whether there is operative temperature control and whether that control is constant or scheduled. A three-value enum is a cleaner way to do this.

Are we still using these constructors? I'm moving away from them incrementally, but should be do a cleanup pass?

If an empty signature constructor isn't doing anything but assigning the values to the member variables directly, then yeah, let's get rid of them. A dedicated pass is fine, and it should be fairly easy to detect them. In fact, a very quick script resulted in the following list of "empty" constructors, which may not be 100% accurate, but my spot checks seem to indicate it is close, with an estimate of 344 to clean up:

Converted enum-to-string functions into constexpr std::array<std::string_view>.

No point in having LowTempRadiant in both module name and type name.

No point in having Control in both type name and type values.

No point in having CondCtrl in both type name and type values.

Constants do not need to be members of the state.data* object.

Another instance where two booleans were used to indicate three states.

When building this branch with Visual Studio on Windows, we encountered a number of strange, nonsensical errors. @mjwitte identified the issue—one of the enum names here, ABSOLUTE, seems to be interpreted by Visual Studio as a function name, which causes the build to fail.
After changing this enum name, the branch builds without any issues. I believe we should rename it to avoid this conflict. If you have a preferred alternative, please let me know. I can go ahead and make the change while also updating the clang format and addressing the issues needed to pass the custom checks. Or if anyone have any insight on how to resolve this without changing the name, please let me know.

Go ahead, thanks!

The only alternative name I can think of is ABSOLUTEVALUE, but if anyone has a better suggestion, feel free to let me know!

It doesn’t matter. Call it Dareum if you want.

😊

If an empty signature constructor isn't doing anything but assigning the values to the member variables directly, then yeah, let's get rid of them. A dedicated pass is fine, and it should be fairly easy to detect them. In fact, a very quick script resulted in the following list of "empty" constructors, which may not be 100% accurate, but my spot checks seem to indicate it is close, with an estimate of 344 to clean up:

Yep, this is nice.

@amirroth On windows, with a debug build, unit test EconomicTariff_LEEDtariffReporting_Test fails with a subscript error here, because distCoolDemWindowUnits=invalid.

@Myoldmopar This fails with a debug build, but not with RelWithDebInfo. I wonder if we need to change the configuration on the CI unit test coverage to Debug?

The RelWithDebInfo was added to make CI run in a reasonable time. It could be changed back, but will result in a pretty major test time penalty. Hmm. I wonder if I can keep RelWithDebInfo, but re-enable subscript errors and assertions? I'll look real quick.

It looks like we could keep the optimizations from a RelWithDebInfo but give up a little performance by re-enabling assertions and subscript checks. I can look into handling NDEBUG and using sanitiziers, but I'm not sure the best path yet. I wonder if @jmarrec would want to look into this?

GCC: define _GLIBCXX_DEBUG. Incidentally, you can just use our option FORCE_DEBUG_ARITHM_GCC_OR_CLANG and this block will be executed

https://github.com/NREL/EnergyPlus/blob/1cfef92bbf108f8c5b5218e917570572154741fd/cmake/CompilerFlags.cmake#L145-L151

On MSVC, I think it's _ITERATOR_DEBUG_LEVEL=2 (which is added by default in Debug mode)

MSVC is a piece of junk when it comes to ABI with this, so you need to ensure all libs are built with the same _ITERATOR_DEBUG_LEVEL or you'll get ABI violations.

ASAN would also catch this I think, but it might be annoying to clean everything up so it doesn't report OTHER things (looking at you, FMI/FMU code). Been a while since a ran a full test suite with ASAN.

Is there something I need to do here?

Yes:

1162: [ RUN      ] EnergyPlusFixture.EconomicTariff_LEEDtariffReporting_Test
1162: /usr/include/c++/13/array:211: constexpr const std::array<_Tp, _Nm>::value_type& std::array<_Tp, _Nm>::operator[](size_type) const [with _Tp = std::basic_string_view<char>; long unsigned int _Nm = 5; const_reference = const std::basic_string_view<char>&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
1/1 Test #1162: EnergyPlusFixture.EconomicTariff_LEEDtariffReporting_Test ...Subprocess aborted***Exception:   

Either in test:

# District cooling
state->dataEconTariff->tariff(3).demandWindow == DemandWindow::Quarter;

# distrinct heating water
state->dataEconTariff->tariff(4).demandWindow == DemandWindow::Quarter;

You probably should assign kindMtr too for meters 3 & 4.

Note that Gas itself has protection in the EconomicTariff.cc while DC /DH don't

https://github.com/NREL/EnergyPlus/blob/1cfef92bbf108f8c5b5218e917570572154741fd/src/EnergyPlus/EconomicTariff.cc#L3959-L3961

Convenience for you: 17aeb29

Decide if you want to also protect in the CC file or not?

I opened #11059, if you can take it from there that'd be great. Have a nice weekend!

Looks like you've already fixed it?