All supported compilers support the builtins, so this removes dead code and simplifies a lot of the functions through that.
Details
Details
- Reviewers
ldionne EricWF - Group Reviewers
Restricted Project - Commits
- rG40d8f87809dc: [libc++] Assume that builtins for math.h functions are available
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/include/math.h | ||
|---|---|---|
| 294 | Mingw is failing the clang-tidy test, and only that. Can you please figure out what's wrong and add the missing tests? | |
| 516 | Those all need to be inline for ODR. | |
| 637 | You removed __promote here and in a bunch of other places. Can you please do that as a separate patch (including the ones that are trivial, i.e. __promote<T> where T is a floating point). That will make this change easier to validate. | |
| 1049 | If the C library does not provide copysign(double, double), this is infinite recursion. This is too subtle and the failure mode is terrible, i.e. this should never be a runtime error. Instead, let's do this (per live review just now): #include <type_traits>
// in C library
float copysignf(float, float);
double copysign(double, double);
long double copysignl(long double, long double);
// in libc++
inline auto copysign(float, float);
inline auto copysign(long double, long double);
template <class = int>
inline auto copysign(double x, double) { return x; }
template <class _Tp, class _Up,
std::enable_if_t<std::is_arithmetic<_Tp>::value && std::is_arithmetic<_Up>::value, int> = 0>
inline auto copysign(_Tp __x, _Up __y);
int main() {
double d = 0;
copysign(d, d);
} | |
Comment Actions
Please start putting the rational in the change description.
How have you verified that these built ins are always provided?
Obviously, this would be less complex and nicer if we could assume their presence, but I have no doubt this more complicated code was written because we couldn't at some point.
Comment Actions
Our compiler requirements have changed a lot since 2012, and nowadays the compilers we support are tested in the CI. I assume @philnik's reasoning was that "I'll try doing it, and if the CI is happy, then it means that all supported compilers are happy". That's what we normally do and it has led to large simplifications (e.g. in how we implement type_traits). I agree it would make sense to record that rationale (or whatever the rationale is in case I'm wrong) in the commit message though.
Comment Actions
It's my understanding that the availability of some of these builtins is a property of what math libraries happen to be on the system that compiled the compiler. As in, if the compiler doesn't have a math library which can perform the computation, it doesn't provide the builtin. Am I incorrect about that?
That would mean that the availability of the builtins is sometimes independent of compiler version.
Comment Actions
I can't find any indication that the builtins are provided conditionally. It also doesn't really make sense, since the compiler could still emit the call, even if the hosting environment doesn't provide the function.
| libcxx/include/math.h | ||
|---|---|---|
| 516 | These are templates, so they are implicitly inline. | |
Comment Actions
Good point. We can ship this and see if anything blows up.
But @ldionnes comments need to be addressed. And are a good example of why changes like this should be scoped to a single type of change.
But this still needs a description before it proceeds.
| libcxx/include/math.h | ||
|---|---|---|
| 1049 | Good spotting. | |
Comment Actions
Good point.
~~~What about -Xclang -fno-math-builtins? Clang provides an option to turn the builtins off, and libc++ currently works when it's specified. We should state why we don't care about supporting this option.~~~
There may be specific logic in clang to ignore -Xclang -fno-math-builtins inside the math.h header. So ignore the question.
But @ldionnes comments need to be addressed. And are a good example of why changes like this should be scoped to a single type of change.
But this still needs a description before it proceeds.
Comment Actions
AFAICT this is still supported. Clang just doesn't add any attributes to the math functions anymore: https://godbolt.org/z/EfKhx896Y
| libcxx/include/math.h | ||
|---|---|---|
| 286 | Can you try to:
If that doesn't make the diff any better, nvm we'll review as-is, but I find it tricky to validate in the current state of the diff. | |
Can you try to:
If that doesn't make the diff any better, nvm we'll review as-is, but I find it tricky to validate in the current state of the diff.