Closed anntzer closed 3 years ago
Also, it's OK to reformat the CMakelist, but it's easier to review if it's a separate PR.
Are you sure you need both variants at the same time ? What's the use-case ? Maybe a cmake option to select the variant is what we need. I dont want to build the lib twice.
Are you sure you need both variants at the same time ? What's the use-case ?
The use case is when cminpack is packaged by redistributors, which include at least linux distros and conda. In that case they will typically just provide a "plain" build of the library, which here means only the double-precision version. I would rather have them provide all precisions together, so that I can easily have whatever precision I need (single, in my case) on machines with no compiler available.
Maybe a cmake option to select the variant is what we need. I dont want to build the lib twice.
Per the above I believe the default should be to provide all of them (because otherwise I'd need to go to each and every redistributor and convince them that they should do a non-default build of the library), but we could reasonably add a CMake flag to just build one of them (now implemented).
@xantares are you satisfied by @anntzer 's replies?
To me it looks OK:
can you rebase and push the changes?
done
Side note: @xantares and @anntzer would you agree to be collaborators on this project? This would allow you to merge PRs, but they would still need to be reviewed and approved by someone else.
@anntzer I'm curious, do you have an automatic formatter for CMakeLists or did you do it by hand?
I did it by hand plus some regexes...
Thanks for the offer of being a collaborator. I don't claim any expertise in the relevant field (I really just needed a reasonable implementation of lmdif
) and already have much on my plate, but can try to occasionally have a look if you need help with something. No time guarantees, though :-)
@devernay Any chance for a new release with these changes? Thanks!
Done!
Thanks!
Closes #41 (per https://github.com/devernay/cminpack/issues/41#issuecomment-622253014). (Only the second commit matters for the feature, but cleaning the style helped me understand the logic :))