MIT-LAE / APCEMM

Aircraft Plume Chemistry, Emissions, and Microphysics Model
MIT License
7 stars 16 forks source link

Fix all compiler warnings #24

Closed lrobion closed 1 month ago

lrobion commented 1 month ago

Fixes all compiler warnings with -Wall. Tested with gcc 14.1 Compiling with -DDEBUG=ON, results are bitwise reproducible with commit fa4fded.

Some warnings should have affected the result but did not because of either not passing a threshold or being cancelled out by another bug.

Added -fopenmp to compile flags as gcc was warning that the OMP #pragma were not defined and ignored otherwise. This seems strange because previous versions of APCEMM did seem to be multithreaded. I don't know if anyone has insights into this.

sdeastham commented 1 month ago

Thanks @lrobion ! This PR changes the compiler requirements, as <format> is part of the C++20 standard instituted in 2020. That header wasn't part of the GNU compiler suite until 2023 (made available with GCC 13) so this constitutes a fairly aggressive change in compatibility. For the moment, can you roll back that requirement please (specifically removing the use of the format header)? Thanks!

speth commented 1 month ago

The CMake config has specified set(CMAKE_CXX_STANDARD 20) since the build system switch, so I'd think C++20 features should be fair game.

An alternative option would be to use {fmt}, which is the library that inspired the introduction std::format. I'd rather not revert to the ugly old string formatting code.

sdeastham commented 1 month ago

I guess I'm specifically sensitive to this one because a couple of the systems I'm working on don't have a recent enough GCC! The {fmt} option sounds good to me.

lrobion commented 1 month ago

I'll switch to using {fmt} for this and see if there are other places that could benefit from it. For the C++20 standard vs compiler support issue, do we want to set a minimum requirement for GCC version and only use the features available there? It'd set a clear line on what is fair game.

Would be good to do the same thing later with clang but I've had issues getting APCEMM to compile altogether.

sdeastham commented 1 month ago

A really good question. To @speth's point, we technically do set a C++ minimum, but that's still not totally clear cut. See for example https://en.cppreference.com/w/cpp/compiler_support which shows that, strictly speaking, no compiler technically fully supports C++20 yet (although it's fair to say that the release of GCC 13 in April last year got them pretty darn close).

I'm generally reluctant to push for recent versions of the GNU compilers because it puts a barrier (not insurmountable) up for users of HPC systems which are not necessarily up to date. I'm interested though to get the perspective from @speth based on Cantera - mine is mostly based on GEOS-Chem and working on systems where I have limited control over modules (basically: I hate trying to rebuild my stack from GCC up). If we can get it to build with GCC 11 I'll be happy (mostly because I can easily test that here at Imperial!) but I'm open to other perspectives for sure.

speth commented 1 month ago

HPC systems often seem to be woefully out of date, and I think we restrict ourselves too much if we based compatibility on what they might have available. At this point, it's usually not that difficult to install an up-to-date compiler with a userspace package manager like Conda (by which I really mean Miniforge) or Spack, and then for APCEMM you can rely on vcpkg for appropriate versions of everything else.

With Cantera, we take a fairly conservative approach to dependency requirements, since it's a library that regularly gets embedded into larger modeling systems. There, we've generally aimed for supporting the two latest Ubuntu LTS releases, which currently means Ubuntu 22.04 and 24.04, where 22.04 has GCC 11.2. So I suppose GCC 11.2 isn't a bad choice for a minimum version. It does seem like we found one of the very last C++20 features that they implemented -- most of the rest of them were available in GCC 11 or even before. {fmt} is a perfectly good alternative, and one we've used in Cantera for quite a while.

sdeastham commented 1 month ago

That works for me! Let's go with GCC 11.2 as the minimum (for now) and {fmt} as our library for non-hideous string processing.

lrobion commented 1 month ago

I've removed std::format calls and replaced them with a dependency on {fmt}.

I ran into some issues linking to the library: Compiling APCEMM with GCC 14.1 with {fmt} linked to the static library worked fine. Doing the same thing with GCC 11.2 leads to a linker error with undefined references in the tests executables. I was not able to fix this (we're not even using fmt in the tests...).

Using {fmt} header-only is the work around for now and works for GCC 11.2. Might be worth looking into if we worry about compile time.