TEOS-10 / GSW-C

C implementation of the Thermodynamic Equation Of Seawater - 2010 (TEOS-10)
Other
18 stars 18 forks source link

Compiler warnings with C++ compilation #77

Open efiring opened 2 weeks ago

efiring commented 2 weeks ago

C++ compilation triggered by the vast number of tests introduced in #71 and #75 yields a flood of warnings in the compilation logs. They come mostly from gsw_check_functions.c; I suspect one or two small changes in that file, repeated in many places, might clear up most of them, but I have not looked closely. There are so many compilations and logs that it is hard to keep track of which runs are clean and which show the warnings, but I think the warnings might be specific to the meson builds--for which the logs don't show the actual compiler invocations.

There are at least 2 questions here: 1) Are there clean and unobtrusive code changes that will lead to warnings-free compilations in all cases of interest? 2) Can we greatly reduce the number of test compilations without losing important information?

peter-urban commented 2 weeks ago

First investigation: 1) The difficult to read warnings are generated in all cases where gcc or clang (or clang-cl on windows) are used to build the tests in c++ mode (the default was that c++ was only used for msvc compiler on windows)

peter-urban commented 2 weeks ago

Ok, I have more PRs in the pipeline to step-by-step resolve 1) and 2). However, they build upon https://github.com/TEOS-10/GSW-C/pull/79 so I will wait until that one is merged.

peter-urban commented 2 weeks ago

@efiring responding to the comment in https://github.com/TEOS-10/GSW-C/pull/80

We could modify the tests to run ninja -v directly (instead of meson compile or meson test) The output would then look like this:

[1/6] gcc -Igsw_check.p -I. -I.. -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -O3 -MD -MQ gsw_check.p/gsw_check_functions.c.o -MF gsw_check.p/gsw_check_functions.c.o.d -o gsw_check.p/gsw_check_functions.c.o -c ../gsw_check_functions.c
[2/6] gcc -Ilibgswteos-10.so.p -I. -I.. -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -O3 -fPIC -MD -MQ libgswteos-10.so.p/gsw_saar.c.o -MF libgswteos-10.so.p/gsw_saar.c.o.d -o libgswteos-10.so.p/gsw_saar.c.o -c ../gsw_saar.c
[3/6] gcc -Ilibgswteos-10.so.p -I. -I.. -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -O3 -fPIC -MD -MQ libgswteos-10.so.p/gsw_oceanographic_toolbox.c.o -MF libgswteos-10.so.p/gsw_oceanographic_toolbox.c.o.d -o libgswteos-10.so.p/gsw_oceanographic_toolbox.c.o -c ../gsw_oceanographic_toolbox.c
[4/6] gcc  -o libgswteos-10.so libgswteos-10.so.p/gsw_oceanographic_toolbox.c.o libgswteos-10.so.p/gsw_saar.c.o -Wl,--as-needed -Wl,--no-undefined -Wl,-O1 -shared -fPIC -Wl,-soname,libgswteos-10.so -lm
[5/6] /ssd/opt/miniforge3/envs/dev/bin/meson --internal symbolextractor /ssd/src/GSW-C-meson/builddir libgswteos-10.so libgswteos-10.so libgswteos-10.so.p/libgswteos-10.so.symbols 
[6/6] gcc  -o gsw_check gsw_check.p/gsw_check_functions.c.o -Wl,--as-needed -Wl,--no-undefined -Wl,-O1 '-Wl,-rpath,$ORIGIN/' -Wl,-rpath-link,/ssd/src/GSW-C-meson/builddir/ -Wl,--start-group libgswteos-10.so -lm -Wl,--end-group

This is after https://github.com/TEOS-10/GSW-C/pull/80, otherwise you'd see the -Wall and -Wextra

Would you prefer this output? If so, I'll create a PR

peter-urban commented 2 weeks ago

https://github.com/TEOS-10/GSW-C/pull/82 and https://github.com/TEOS-10/GSW-C/pull/83 should resolve the remaining warnings

peter-urban commented 2 weeks ago

Thx for merging. If I didn't miss anything, the warnings are now resolved. Next step in this issue would be to further simplify the test matrix.

a) One possibility would be to not test shared and static libraries for all configurations. Maybe shared is enough for most cases? Static will likely work if shared does.

b) Additionally: renaming the test cases may help to increase the overview. Suggestions welcome.

I'll look into these 2 steps, but I'll first have to focus on other projects again, so it may take some time.

peter-urban commented 2 weeks ago

Additionally, we could consider forcing the compiler to treat warnings as errors? This way we wouldn't need to check the logs, unless there is a failure?

efiring commented 2 weeks ago

No rush at all for additional changes. Mostly static only or shared only: I think this makes sense. Warnings as errors: That would probably work out OK, given that we are starting with no warnings.