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

Correctly namespace more math functions. #39666

Merged
merged 1 commit into from
Apr 20, 2020

Conversation

kevingranade
Copy link
Member

Summary

SUMMARY: None

Purpose of change

As outlined in #39388 these non-standard invocations of math functions are problematic.

Describe the solution

Stick em in the std namespace.

Testing

Needs to build, and I expect lgtm to agree it cleared up a bunch of alerts.

Additional context

I realized something else, implementations are allowed to put e.g. pow() in the global namespace in , but they are also allowed not to, which would fail our build on such a system.

@ZhilkinSerg ZhilkinSerg added [C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments labels Apr 18, 2020
@kevingranade kevingranade force-pushed the math-fun branch 4 times, most recently from 0ccd019 to d9b0677 Compare April 18, 2020 18:50
@kevingranade
Copy link
Member Author

Weird roadbump, GCC standard library does not export sqrtf() into the std namespace.
https://stackoverflow.com/questions/43183252/cmake-sqrtf-is-not-a-member-of-std

@jbytheway
Copy link
Contributor

Weird roadbump, GCC standard library does not export sqrtf() into the std namespace.
https://stackoverflow.com/questions/43183252/cmake-sqrtf-is-not-a-member-of-std

Yeah, I remember running into this issue when I looked into some related warnings. I didn't come up with a good answer.

@kevingranade
Copy link
Member Author

AFAICT the answer is either just don't use it or export it into std ourselves.
I really don't like the latter because it essentially means we have to carry around our own cmath wrapper and use it everywhere.

@kevingranade kevingranade force-pushed the math-fun branch 2 times, most recently from 6209268 to 30960c2 Compare April 18, 2020 20:22
@jbytheway
Copy link
Contributor

Agreed. I think just don't use it. It will lead to more -Wdouble-promotion errors errors if we ever turn on that particular warning, but that's life.

@kevingranade
Copy link
Member Author

Turns out same story with powf() >_<

@lgtm-com
Copy link

lgtm-com bot commented Apr 18, 2020

This pull request fixes 80 alerts when merging 30960c2 into a664636 - view on LGTM.com

fixed alerts:

  • 80 for Use of c-style math functions

@lgtm-com
Copy link

lgtm-com bot commented Apr 19, 2020

This pull request fixes 79 alerts when merging 7b588d3 into 388be3b - view on LGTM.com

fixed alerts:

  • 79 for Use of c-style math functions

@lgtm-com
Copy link

lgtm-com bot commented Apr 19, 2020

This pull request fixes 79 alerts when merging fa2cd6b into 72a6afa - view on LGTM.com

fixed alerts:

  • 79 for Use of c-style math functions

@ZhilkinSerg ZhilkinSerg merged commit 0a849e2 into CleverRaven:master Apr 20, 2020
@kevingranade kevingranade deleted the math-fun branch June 28, 2020 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants