Fix #11179 - re-enable warnings gcc clang and turn on `-Werror` by jmarrec · Pull Request #11180 · NatLabRockies/EnergyPlus

jmarrec · 2025-08-28T00:09:38Z

Pull request overview

Fixes Warnings are disabled on gcc/clang (-w flag is leaking from third_party) #11179

Description of the purpose of this PR

Fixup third_party code to avoid leaking the -w flag
Turn on Werror
Fixup all errors until it compiles (at least on GCC 13.2 in Release)

I built with -fdiagnostics-format=json before turning on Werror, and parsed the output.

ninja &> diagnostics

I found 1496 warnings, 296 unique.

In the unique ones, classification counts were

Counter({'-Wunused-variable': 192,
         '-Wstrict-aliasing': 35,
         '-Wunused-parameter': 21,
         '-Wsign-compare': 15,
         '-Wswitch': 13,
         '-Wunused-value': 9,
         '-Wuninitialized': 5,
         '-Wreorder': 3,
         '-Wparentheses': 2,
         '-Wsequence-point': 1})

Pull Request Author

Reviewer

jmarrec · 2025-08-28T00:11:36Z

third_party/CMakeLists.txt

 add_subdirectory(FMI)
 set_target_properties(epfmiimport PROPERTIES FOLDER ThirdParty/FMI)
-target_link_libraries(epfmiimport turn_off_warnings)
+target_link_libraries(epfmiimport PRIVATE turn_off_warnings)


that was the root cause of the -w flag leak

jmarrec · 2025-08-28T00:12:23Z

third_party/FMI/CMakeLists.txt

+endif()

-TARGET_LINK_LIBRARIES( epfmiimport epexpat miniziplib )
+target_link_libraries(epfmiimport PRIVATE epexpat miniziplib)


Couldn't mix the old signature with the new that has PUBLIC/PRIVATE/INTERFACE, and probably why it was the only target linking to turn_off_warnings that wasn't linking with PRIVATE

jmarrec · 2025-08-28T00:13:15Z

src/EnergyPlus/OutputProcessor.hh

+// A helper to be able to cast an enum to an int& for use in the above function
+template <typename EnumType, typename = std::enable_if_t<std::is_enum_v<EnumType>>>
+void SetupOutputVariable(EnergyPlusData &state,
+                         std::string_view const VariableName,                                 // String Name of variable
+                         Constant::Units VariableUnit,                                        // Actual units corresponding to the actual variable
+                         EnumType &ActualVariable,                                                 // Actual Variable, used to set up pointer
+                         OutputProcessor::TimeStepType TimeStepType,                          // Zone, HeatBalance=1, HVAC, System, Plant=2
+                         OutputProcessor::StoreType VariableType,                             // State, Average=1, NonState, Sum=2
+                         std::string const &KeyedValue,                                       // Associated Key for this variable
+                         int const indexGroupKey = -999,                                      // Group identifier for SQL output
+                         OutputProcessor::ReportFreq freq = OutputProcessor::ReportFreq::Hour // Internal use -- causes reporting at this freqency
+        ) {
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wstrict-aliasing"
+    SetupOutputVariable(state,
+                        VariableName,
+                        VariableUnit,
+                        (int &)ActualVariable,
+                        TimeStepType,
+                        VariableType,
+                        KeyedValue,
+                        indexGroupKey,
+                        freq);
+#pragma GCC diagnostic pop
+};


This is to silence the Wstrict-aliasing warnings

/home/julien/Software/Others/EnergyPlus/src/EnergyPlus/EMSManager.cc: In function ‘void EnergyPlus::EMSManager::SetupPrimaryAirSystemAvailMgrAsActuators(EnergyPlus::EnergyPlusData&)’: /home/julien/Software/Others/EnergyPlus/src/EnergyPlus/EMSManager.cc:1639:84: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing] 1639 | (int &)state.dataAirLoop->PriAirSysAvailMgr(Loop).availStatus); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~

jmarrec · 2025-08-28T00:14:15Z

src/EnergyPlus/EMSManager.cc

+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wstrict-aliasing"
                SetupEMSActuator(state,
                                 "AirLoopHVAC",
                                 state.dataAirSystemsData->PrimaryAirSystems(Loop).Name,
                                 "Availability Status",
                                 "[ ]",
                                 state.dataEMSMgr->lDummy2,
                                 (int &)state.dataAirLoop->PriAirSysAvailMgr(Loop).availStatus);
+#pragma GCC diagnostic pop


This is the only call to SetupEMSActuator that was violating strict-aliasing, so just pragma around it, no need for a template

jmarrec · 2025-08-28T00:15:18Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

                Real64 HGround = 0.0;   // "Convection" coefficient from ground to surface
                Real64 HAir = 0.0;      // "Convection" coefficient from air to surface (radiation)
-                Real64 HSurrr = 0.0;    // "Linearized radiation" coefficient from surrounding surfaces to surface
+                // Real64 HSurrr = 0.0;    // "Linearized radiation" coefficient from surrounding surfaces to surface


The rest is mostly fixing some unused variables or parameters and a couple of weird things

jmarrec · 2025-08-28T00:15:44Z

src/EnergyPlus/DataSizing.cc

                if ((spaceNum == 0) || (spaceNum == dsoaSpaceNum)) {
                    Real64 spaceArea = state.dataHeatBal->space(this->dsoaSpaceIndexes(dsoaCount)).FloorArea;
-                    sumArea + -spaceArea;
+                    sumArea += spaceArea;


It did catch a bunch of scary looking problems!

Yeah, how did it not cause diffs!?

jmarrec · 2025-08-28T00:16:06Z

src/EnergyPlus/GroundHeatExchangers.cc


    // Add the rest of the long time-step g-functions to the combined curve
-    for (int index_longTS = 0; index_longTS < this->myRespFactors->GFNC.size(); ++index_longTS) {
+    for (size_t index_longTS = 0; index_longTS < this->myRespFactors->GFNC.size(); ++index_longTS) {


Wsign-compare

jmarrec · 2025-08-28T00:16:13Z

src/EnergyPlus/GroundHeatExchangers.cc

    }

-    for (int i = 1; i < g.size() - 1; ++i) {
+    for (int i = 1; i < static_cast<int>(g.size() - 1); ++i) {


another Wsign-compare

jmarrec · 2025-08-28T13:18:47Z

src/EnergyPlus/EconomicTariff.cc

        Op op = static_cast<Op>(getEnumValue(opNamesUC, word));
        if (op == Op::Invalid) {
-            static_cast<Op>(getEnumValue(opNames2UC, word));
+            op = static_cast<Op>(getEnumValue(opNames2UC, word));
        }


This is one of the cases where I could see a potential diff in the regressions. It was clearly wrong before

Myoldmopar · 2025-08-28T15:46:00Z

WOW! And just like that, EnergyPlus is at -Werror and you've addressed hundreds of warnings that were being silenced. AND NO DIFFS!! This is fantastic, @jmarrec. I'll take a look at the changes, but I'm confident this will drop in right away.

Myoldmopar

This is fantastic! I approve of the changes, although I'd like to add in a Windows build to the PR before I merge it into develop. I guess alternatively I could kick off a release package build which would do all three platforms. I will hold for now, but I'm happy to do whatever is needed to wrap this up, it is great.

Myoldmopar · 2025-08-28T15:47:09Z

cmake/CompilerFlags.cmake


  # COMPILER FLAGS
  target_compile_options(project_options INTERFACE -pipe) # Faster compiler processing
+  target_compile_options(project_options INTERFACE -Werror) # Compiler Driven Development


Myoldmopar · 2025-08-28T15:47:35Z

src/EnergyPlus/Coils/CoilCoolingDX.cc

                        "Cooling Coil Dehumidification Mode",
                        Constant::Units::None,
-                        (int &)this->dehumidificationMode,
+                        this->dehumidificationMode,


Yes, very good!

Myoldmopar · 2025-08-28T15:48:30Z

src/EnergyPlus/Plant/EquipAndOperations.cc

                   (this->Report.BuildingPolledHeatingLoad > HVAC::SmallLoad)) {
            this->PlantOps.AirSourcePlantSimultaneousHeatingAndCooling = true;
-            if (this->Report.BuildingPolledHeatingLoad > abs(this->Report.BuildingPolledCoolingLoad)) {
+            if (this->Report.BuildingPolledHeatingLoad > std::abs(this->Report.BuildingPolledCoolingLoad)) {


A great find, and I'm glad it didn't cause any diffs.

Myoldmopar · 2025-08-28T15:48:55Z

src/EnergyPlus/DataSizing.cc

                if ((spaceNum == 0) || (spaceNum == dsoaSpaceNum)) {
                    Real64 spaceArea = state.dataHeatBal->space(this->dsoaSpaceIndexes(dsoaCount)).FloorArea;
-                    sumArea + -spaceArea;
+                    sumArea += spaceArea;


Yeah, how did it not cause diffs!?

jmarrec · 2025-09-01T09:24:20Z

This is fantastic! I approve of the changes, although I'd like to add in a Windows build to the PR before I merge it into develop. I guess alternatively I could kick off a release package build which would do all three platforms. I will hold for now, but I'm happy to do whatever is needed to wrap this up, it is great.

I've launched a test release at https://github.com/NREL/EnergyPlus/releases/tag/v25.2.0-TestWerror

```diff build third_party/ssc/splinter/CMakeFiles/splinter.dir/mykroneckerproduct.cpp.o: CXX_COMPILER__splinter_unscanned_Release /home/julien/Software/Others/EnergyPlus/third_party/ssc/splinter/mykroneckerproduct.cpp || cmake_object_order_depends_target_splinter CONFIG = Release + DEFINES = -DNDEBUG -D__64BIT__ -D__UNIX__ DEP_FILE = third_party/ssc/splinter/CMakeFiles/splinter.dir/mykroneckerproduct.cpp.o.d - FLAGS = -O3 -DNDEBUG -std=gnu++11 -fPIC -fvisibility=hidden -D__64BIT__ -D__UNIX__ -O3 -DNDEBUG -w -Wno-error -w + FLAGS = -O3 -DNDEBUG -std=gnu++11 -fPIC -fvisibility=hidden -w -Wno-error INCLUDES = -I/home/julien/Software/Others/EnergyPlus/third_party/ssc/splinter/. -I/home/julien/Software/Others/EnergyPlus/third_party/ssc/splinter/../ssc LAUNCHER = /usr/bin/ccache OBJECT_DIR = third_party/ssc/splinter/CMakeFiles/splinter.dir @@ -934,8 +943,9 @@ ``` No more duplicates.

…TE, so it was treated publicly ```diff --- build.ninja.bak2 2025-08-27 20:52:22.434771013 +0200 +++ build.ninja 2025-08-27 20:52:25.455853497 +0200 @@ -9302,7 +9302,7 @@ CONFIG = Release DEFINES = -DEP_cache_GlycolSpecificHeat -DEP_psych_errors -DHAVE_FEENABLEEXCEPT -DJSON_USE_IMPLICIT_CONVERSIONS=1 -DLINK_WITH_PYTHON -DOBJEXXFCL_ALIGN=64 -DXML_STATIC DEP_FILE = src/EnergyPlus/CMakeFiles/energypluslib.dir/cmake_pch.hxx.gch.d - FLAGS = -O3 -DNDEBUG -fPIC -fvisibility=hidden -pipe -fno-stack-protector -fdiagnostics-color=always -Wno-deprecated-declarations -w -fPIC -Wpedantic -Wall -Wextra -Wno-unknown-pragmas -Wno-deprecated-copy -Wno-attributes -Wno-delete-non-virtual-dtor -Wno-missing-braces -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -Wno-maybe-uninitialized -Wno-aggressive-loop-optimizations -Wno-dangling-reference -Wno-array-bounds -Wno-stringop-overflow -Winvalid-pch -x c++-header -include /home/julien/Software/Others/EnergyPlus-build-release/src/EnergyPlus/CMakeFiles/energypluslib.dir/cmake_pch.hxx + FLAGS = -O3 -DNDEBUG -fPIC -fvisibility=hidden -pipe -fno-stack-protector -fdiagnostics-color=always -Wno-deprecated-declarations -fPIC -Wpedantic -Wall -Wextra -Wno-unknown-pragmas -Wno-deprecated-copy -Wno-attributes -Wno-delete-non-virtual-dtor -Wno-missing-braces -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -Wno-maybe-uninitialized -Wno-aggressive-loop-optimizations -Wno-dangling-reference -Wno-array-bounds -Wno-stringop-overflow -Winvalid-pch -x c++-header -include /home/julien/Software/Others/EnergyPlus-build-release/src/EnergyPlus/CMakeFiles/energypluslib.dir/cmake_pch.hxx INCLUDES = -I/home/julien/.pyenv/versions/3.12.2/include/python3.12 -I/home/julien/Software/Others/EnergyPlus/idd -I/home/julien/Software/Others/EnergyPlus/third_party/fmt-8.0.1/include -I/home/julien/Software/Others/EnergyPlus/src -I/home/julien/Software/Others/EnergyPlus/third_party -I/home/julien/Software/Others/EnergyPlus/third_party/btwxt/include -I/home/julien/Software/Others/EnergyPlus/third_party/re2 -I/home/julien/Software/Others/EnergyPlus/third_party/doj -I/home/julien/Software/Others/EnergyPlus/third_party/nlohmann -I/home/julien/Software/Others/EnergyPlus/third_party/fast_float -I/home/julien/Software/Others/EnergyPlus/third_party/valijson -I/home/julien/Software/Others/EnergyPlus/third_party/ObjexxFCL/src -I/home/julien/Software/Others/EnergyPlus/third_party/CLI -I/home/julien/Software/Others/EnergyPlus/third_party/eigen -I/home/julien/Software/Others/EnergyPlus/third_party/Windows-CalcEngine/src/Chromogenics/include -I/home/julien/Software/Others/EnergyPlus/third_party/Windows-CalcEngine/src/Common/include -I/home/julien/Software/Others/EnergyPlus/third_party/Windows-CalcEngine/src/Gases/include -I/home/julien/Software/Others/EnergyPlus/third_party/Windows-CalcEngine/src/MultiLayerOptics/include -I/home/julien/Software/Others/EnergyPlus/third_party/Windows-CalcEngine/src/SingleLayerOptics/include -I/home/julien/Software/Others/EnergyPlus/third_party/Windows-CalcEngine/src/SpectralAveraging/include -I/home/julien/Software/Others/EnergyPlus/third_party/Windows-CalcEngine/src/Tarcog/include -I/home/julien/Software/Others/EnergyPlus/third_party/Windows-CalcEngine/src/Viewer/include -I/home/julien/Software/Others/EnergyPlus/third_party/ssc -I/home/julien/Software/Others/EnergyPlus/third_party/penumbra/include -I/home/julien/Software/Others/EnergyPlus/third_party/kiva/src -I/home/julien/Software/Others/EnergyPlus-build-release/third_party/kiva/src/libkiva -I/home/julien/Software/Others/EnergyPlus/third_party/cpgfunctionEP -I/home/julien/Software/Others/EnergyPlus/third_party/cpgfunctionEP/include -I/home/julien/Software/Others/EnergyPlus/third_party/kiva/src/libkiva -I/home/julien/Software/Others/EnergyPlus/third_party/kiva/vendor/boost-1.77.0 -I/home/julien/Software/Others/EnergyPlus/third_party/kiva/vendor/eigen -I/home/julien/Software/Others/EnergyPlus/third_party/btwxt/vendor/courierr/include -I/home/julien/Software/Others/EnergyPlus/src/EnergyPlus/AirflowNetwork/include -I/home/julien/Software/Others/EnergyPlus/third_party/ssc/jsoncpp/src/lib_json/../../include -I/home/julien/Software/Others/EnergyPlus-build-release/third_party/ssc/jsoncpp/include/json -I/home/julien/Software/Others/EnergyPlus/third_party/libtk205/include/libtk205 -I/home/julien/Software/Others/EnergyPlus/third_party/libtk205/vendor/btwxt/include -isystem /home/julien/Software/Others/EnergyPlus/third_party/SQLite -isystem /home/julien/Software/Others/EnergyPlus/third_party/Expat -isystem /home/julien/Software/Others/EnergyPlus/third_party/Expat/lib -isystem /home/julien/Software/Others/EnergyPlus/third_party/kiva/vendor/boost-1.61.0 -isystem /home/julien/Software/Others/EnergyPlus/nlohmann LAUNCHER = /usr/bin/ccache OBJECT_DIR = src/EnergyPlus/CMakeFiles/energypluslib.dir ```

…se for src/EnergyPlus

…ICT_ALIASING changes

…ilences the Wstrict-aliasing

…ict-aliasing

…t is taken out in Release)

``` /home/julien/Software/Others/EnergyPlus2/src/EnergyPlus/EMSManager.cc:2022:32: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move] 2022 | auto tup = std::make_tuple(std::move(Util::makeUPPER(objType)), std::move(Util::makeUPPER(objName)), std::move(Util::makeUPPER(controlTypeName))); ```

/home/julien/Software/Others/EnergyPlus2/third_party/FMUParser/fmumini.c:449:5: error: a function definition without a prototype is deprecated in all versions of C and is not supported in C23 [-Werror,-Wdeprecated-non-prototype] 449 | int do_extract_onefile(uf, filename, opt_extract_without_path, opt_overwrite, password) unzFile uf;

energypluslib is error free on clang 18

…) (be safe)

…an't remove, mark maybe_unused (locally I built clang in Debug)

This reverts commit 51f6e7c.

jmarrec · 2025-09-01T13:26:48Z

I broken windows by touching the SSC stuff. I know how to fix it, but I think I'm just going to revert the changes instead. I assume changing the SSC cmake here will make updating SSC with upstream more complicated.

The issue is that I changed, ssc/CMakeLists.txt:

function(set_additional_compile_options target options) 
-    get_target_property(MAIN_CFLAGS ${target} COMPILE_FLAGS)
-    if (MAIN_CFLAGS MATCHES "NOTFOUND")
-        set(MAIN_CFLAGS "")
-    endif()
-    set(MAIN_CFLAGS "${MAIN_CFLAGS} ${options}")
-    set_target_properties(${target} PROPERTIES COMPILE_FLAGS ${MAIN_CFLAGS})
+    target_compile_options(${target} PRIVATE ${options})
endfunction()

Then in ssc/splinter/CMakeLists.txt, which I DID NOT modify:

if(MSVC)
   set_additional_compile_options(splinter "/W2 /wd4267 /wd4244")
endif(MSVC)

As a result the STRING "/W2 /wd4267 /wd4244" is passed directly and verbatim to cl.exe.

I would modify that to target_compile_options(splinter PRIVATE /W2 /wd4267 /wd4244) and it'd work, but I have to do that in 5 different CMakeLists.txt

jmarrec · 2025-09-01T13:30:44Z

Launched another release at https://github.com/NREL/EnergyPlus/releases/tag/v25.2.0-TestWerror (after deleting previous one)

jmarrec · 2025-09-01T14:35:26Z

@Myoldmopar all green

Myoldmopar · 2025-09-01T15:53:23Z

Yes let's go! Thanks @jmarrec, awesome stuff here. Merging this now.

jmarrec self-assigned this Aug 28, 2025

jmarrec added Defect Includes code to repair a defect in EnergyPlus Developer Issue Related to cmake, packaging, installers, or developer tooling (CI, etc) labels Aug 28, 2025

jmarrec changed the title ~~FIx #11179 - reeneable warnings gcc clang and turn on -Werror~~ Fix #11179 - re-enable warnings gcc clang and turn on -Werror Aug 28, 2025

jmarrec commented Aug 28, 2025

View reviewed changes

jmarrec requested a review from Myoldmopar August 28, 2025 00:25

jmarrec commented Aug 28, 2025

View reviewed changes

Myoldmopar approved these changes Aug 28, 2025

View reviewed changes

jmarrec added 20 commits September 1, 2025 15:23

Modernize FMI CMakeLists.txt while at it

23c8424

Enable -Werror -Wno-error=strict-aliasing and fix everything in Relea…

3bf0027

…se for src/EnergyPlus

Werror enabled on tst!

c3a059a

Fixup a test

126f0e3

Re-enable error on strict-aliasing and revert the DISABLE_WARNING_STR…

1ebc35d

…ICT_ALIASING changes

Add a templated SetupOutputVariable that accepts a std::is_enum and s…

f5b3023

…ilences the Wstrict-aliasing

Shush the call to SetupEMSActuator for AvailStatus that triggers Wstr…

d89f882

…ict-aliasing

Lint/Format

2cc14a9

Fix one thing my GCC didn't warn on Release + 2 found in debug (asser…

565e843

…t is taken out in Release)

Keep on solving Werror on clang 18 (lots of -Wunused-const-variable)

3e8f243

pessimizing move + a potential issue in Economic Tariff (review!)

428f200

energypluslib is error free on clang 18

Werror ok on tests with clang18 (Debug)

9995732

pragma diangostics ONLY if #if defined(__GNUC__) || defined(__clang__…

8a0ae4a

…) (be safe)

CI catches stuff that works locally..

6a81e5a

CI catches the unused because it's Release, and the assert is gone. C…

70ec79d

…an't remove, mark maybe_unused (locally I built clang in Debug)

Another CI error I don't get locally

edfe37e

jmarrec added 2 commits September 1, 2025 15:23

Don't hardcode nrpoc in workflow

d780906

Revert "Modernize ssc cmake - still showing -w"

b1b8ac1

This reverts commit 51f6e7c.

jmarrec force-pushed the 11179_Reeneable_Warnings_gcc_clang branch from ab0b5c5 to b1b8ac1 Compare September 1, 2025 13:28

Myoldmopar merged commit 89f2dd2 into develop Sep 1, 2025
24 checks passed

Myoldmopar deleted the 11179_Reeneable_Warnings_gcc_clang branch September 1, 2025 15:53

jmarrec mentioned this pull request Sep 5, 2025

Windows fixup for current develop #11191

Merged

20 tasks

Conversation

jmarrec commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request overview

Description of the purpose of this PR

Pull Request Author

Reviewer

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Myoldmopar commented Aug 28, 2025

Uh oh!

Myoldmopar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec commented Sep 1, 2025

Uh oh!

jmarrec commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarrec commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarrec commented Sep 1, 2025

Uh oh!

Myoldmopar commented Sep 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmarrec commented Aug 28, 2025 •

edited

Loading

jmarrec commented Sep 1, 2025 •

edited

Loading

jmarrec commented Sep 1, 2025 •

edited

Loading