Improve single-precision support.

devernay / cminpack

A C/C++ rewrite of the MINPACK software (originally in FORTRAN) for solving nonlinear equations and nonlinear least squares problems

http://devernay.free.fr/hacks/cminpack/

145 stars 63 forks source link

Improve single-precision support. #47

Closed anntzer closed 3 years ago

anntzer commented 3 years ago

The first commit switches from using math.h to tgmath.h. This means that the single precision variants will use e.g. sqrtf instead of casting to double, calling sqrt, and casting back to float. Hopefully this should be slighly faster, although I have admittedly not performed any profiling. This also makes the manual definition of #define foo(x) fool(x) for the longdouble case unnecessary. (Instead of using tgmath.h I could also have use #define foo(x) foof(x) in the float case, but tgmath.h seemed completely suitable for this use case...)

I made sure that nothing was missed by compiling with gcc's -Wconversion option. In the second commit I added a few more manual casts to suppress other instances of -Wconversion.

devernay commented 3 years ago

I'm ready to merge this, but the appveyor build fails. Can you check how to get tgmath.h on VS2015?

devernay commented 3 years ago

tgmath.h is actually C11. Can we keep the code compatible with C99?

https://docs.microsoft.com/en-us/cpp/c-runtime-library/tgmath?view=msvc-160

Are Linux libraries supposed to be using only C99, or is C11 OK on Linux (@xantares ?). Is it OK on any Linux/glibc version? Will this prevent using the library from C99 code?
cmake should set the proper flags to compile with C11 (for VS2015, it's /std:c11). There must be a similar flag for clang/gcc
the inclusion of tgmath.h instead of math.h, as well as the long double section removal should be triggered by #if __STDC_VERSION__ >= 201112L

anntzer commented 3 years ago

Interestingly enough, cppreference says that tgmath.h is C99. Well, I can try to use the #define foo(x) foof(x) approach instead...

devernay commented 3 years ago

Hum yeah this is ambiguous. Maybe all we need is to add C99 flags to the build? can you try that instead?

devernay commented 3 years ago

https://cmake.org/cmake/help/latest/prop_tgt/C_STANDARD.html or better: https://cmake.org/cmake/help/latest/prop_tgt/C_STANDARD_REQUIRED.html

anntzer commented 3 years ago

Actually the #define approach is better because you catch even more places where floats got accidentally promoted to doubles (tgmath silently calls the double version for you).

coveralls commented 3 years ago

Coverage remained the same at 94.105% when pulling 011bd4d353dfbf124c831efe899acf4ebb1d1726 on anntzer:single into d173c7a743cc51309d677683c4a32662336cd993 on devernay:master.