Skip to content

Move to C++17 #164

Description

@HugoFara

CMakeLists.txt:59 sets CMAKE_CXX_STANDARD 11 with STANDARD_REQUIRED ON. The build is now actively downgrading the compiler: GCC 16 reports __cplusplus 202002L by default, so ForeFire opts out of C++20 to opt into C++11.

It already builds and passes under C++17 and C++20, unchanged — no source changes, no shims:

Standard Build Unit tests runff
C++17 0 errors
C++20 0 errors 22/22, 2115 assertions KML + NetCDF pass

The bump is a two-line diff; the work is what it enables.

This needs a decision from @filippi

Discussed with @antonio-leblanc, who sees no obstacle. The person who can be impacted is @filippi, who may have pipelines pinned to C++11 — HPC sites and Meso-NH-adjacent builds are where an older toolchain is likely. This should not land without their agreement. What would help: which machines those pipelines run on, and the oldest compiler among them. GCC 5+ supports C++17 in full, so the bar is low, but the repository cannot answer this.

Meso-NH itself does not constrain the standard. It compiles no ForeFire C++ at all — it builds one C file (C_ForeFire_Interface.c), dlopens libForeFire.so, and resolves 17 extern "C" symbols through dlsym. There is no -std=c++ anywhere in the Meso-NH tree on 5.6 or 6.0. The build environments it runs in might still constrain it; the coupling does not.

The payoff is memory ownership

#159, #160 and the ownership gap behind #162's 5 MB of reported leaks are all one thing: raw owning pointers with no destructor discipline, across 200+ new/delete sites. unique_ptr for session- and domain-owned objects removes the class of bug rather than fixing instances. The session refactor keeps colliding with exactly this.

Also unlocked: std::optional for the SimulationParameters sentinel, currently the magic string "1234567890" (SimulationParameters.cpp:22); [[maybe_unused]] for a large share of #161's 314 -Wunused-parameter; std::filesystem for the path normalisation the HTTP path-traversal fix needs; structured bindings and if-init for the map lookups in DataBroker and Command.

17 rather than 20 is the conservative choice for a code that builds on HPC sites, and delivers all of the above. Nothing here argues against 20 later.

Suggested order

After #161, so the two diffs do not mix — the standard bump will itself change which warnings fire. Then convert ownership incrementally, starting with the objects Command::quit and ~FireDomain already try to manage.


Drafted by Claude Opus 5 from a codebase audit. Reviewed by a maintainer before filing.

EDIT: rewrote for human readability.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions