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

Do we really need to check if pow exists in CMake? #1043

Open
BenjamenMeyer opened this issue Feb 27, 2025 · 10 comments
Open

Do we really need to check if pow exists in CMake? #1043

BenjamenMeyer opened this issue Feb 27, 2025 · 10 comments

Comments

@BenjamenMeyer
Copy link
Member

While working on #1041 @royfalk asked:

I was unable to find a lot of information about this. Is there really a chance modern systems don't have POW? (whatever that is)
I'm asking because a lot of stuff in our cmake was really legacy from 2001.

Back in 2000 it was probably more common that pow and other math functions might not exist. But is that really true today? Could we get rid of these checks?

vincele added a commit to vincele/Vega-Strike-Engine-Source that referenced this issue Mar 2, 2025
@stephengtuggy
Copy link
Contributor

We need pow. This block of code tells us where to find it (i.e., what library to link with). I don't think this block should be removed.

@stephengtuggy
Copy link
Contributor

I think the reason for the IF might be to avoid duplicating pow-detection logic if we've already done that work in a previous cmake run.

@royfalk
Copy link
Contributor

royfalk commented Mar 4, 2025

Honestly, I don't get this. According to cppreference.com, pow has been around since forever really. It's part of the standard library. Are you telling me there's a compiler in 2025 that doesn't support it?!
cppreference doesn't even mention when it was introduced. It just says pre-2011.

@vincele
Copy link
Contributor

vincele commented Mar 4, 2025

I think I've read somewhere that there's libms that are automatically added by compilers, but not all compilers do/did that, but that was a long time ago... That may have been a reason for this kind of manual handling.

But I really don't know. I created the PR to see if the CI caught something wrong, but it didn't.

So how can I dig deeper ?

Do we care about what is not covered by the CI ?
If there's something that break, and someone care, we'll get a report and fix it then...

Anyways, I can drop the PR if this piece of cmake magic is deemed important enough.

And BTW I saw we already have some std::pow() uses somewhere, is that the same pow() as the one checked here ?

@stephengtuggy
Copy link
Contributor

@vincele Good questions. I don't know the answers, unfortunately.

Perhaps one thing we could check for is fallback code that uses a different method of calculating exponents if pow() is not found. If there isn't anything of that sort, then maybe we can just use std::pow() across the board; get rid of this CMake logic; and call it good. After making sure it passes CI and play-testing, of course.

@BenjamenMeyer
Copy link
Member Author

yes we probably should rely on std::pow() now and make it clear that that is what we are doing.

@vincele
Copy link
Contributor

vincele commented Mar 9, 2025

OK, I'll try the full std::pow migration, and we'll see what the CI will tell us...

@royfalk
Copy link
Contributor

royfalk commented Mar 9, 2025

Am I missing something? I assume calling pow and std::pow would call the same code, just as a C vs C++ api.

vincele added a commit to vincele/Vega-Strike-Engine-Source that referenced this issue Mar 9, 2025
@vincele
Copy link
Contributor

vincele commented Mar 9, 2025

That was one of my questions above, that shows I'm more a C than a C++ guy...

vincele added a commit to vincele/Vega-Strike-Engine-Source that referenced this issue Mar 9, 2025
@BenjamenMeyer
Copy link
Member Author

Am I missing something? I assume calling pow and std::pow would call the same code, just as a C vs C++ api.

that depends. If you do:

using namespace std;

then it could match either one since it would be at the compiler's discretion of which to link against based on which it found.
if we do not have that, then a call to pow would have to match our implementation if provided or the C API unless we explicitly say std::pow then it would only match the C++ Standard Library's implementation.

This is is one reason why it's good practice to not import non-local namespaces in C++. It's may be more text to type std:: all the time but it maintains clarity of where something is coming from.

Header files should never have using namespace in them.
IMHO, implementation files should only have a using namespace to match the sole header file it is implementing, not for others so that this isn't an issue.

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

No branches or pull requests

4 participants