Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cmake - Remove pow & some libm & math.h handling #1051

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vincele
Copy link
Contributor

@vincele vincele commented Mar 2, 2025

Fixes issue: #1043

Code Changes:

Issues:

  • PR'ed to let CI run on different systems, was OK on void-linux, x86_64, musl libc

Purpose:

  • What is this pull request trying to do? Removing old cruft from cmake config
  • What release is this for? Next one
  • Is there a project or milestone we should apply this to? Next one

@stephengtuggy
Copy link
Contributor

I'm not so sure this is a good idea. I suspect there was a good reason why this block was there, and the reason probably still applies.

@vincele
Copy link
Contributor Author

vincele commented Mar 3, 2025

Still it built on all the CI platforms (BTW, there's no BeOS in there, but having this diversity (different linuxes, macos & windows is really good !).

I think pow() is such a basic math function that missing it would be fairly obvious, I don't think this would induce subtle bugs. But with FP math, we never know what lurks in there...

@royfalk
Copy link
Contributor

royfalk commented Mar 4, 2025

Let's keep the discussion in #1043.

@royfalk
Copy link
Contributor

royfalk commented Mar 6, 2025

@BenjamenMeyer can you add your two cents here?
I'd prefer to reach some consensus here if possible.

In short, @vincele and I think pow checks can be removed.
The C++ documentation thinks this has been a part of the standard since a long time ago. Possibly C++03. Certainly Pre C++11.
@stephengtuggy advises caution.

@BenjamenMeyer
Copy link
Member

I advise caution for the time being while we confirm platforms.

Let me get #1041 finished & merged, and we can test and do some research on this in the meantime.

@vincele vincele force-pushed the cmake_remove_pow branch from 42465e7 to bd846de Compare March 9, 2025 08:57
@vincele vincele force-pushed the cmake_remove_pow branch 2 times, most recently from 44f20dd to d7ef5f9 Compare March 9, 2025 10:22
Copy link
Member

@BenjamenMeyer BenjamenMeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking this per #1051 (comment) until I at least get that work done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants