[libcxx-commits] [PATCH] D135781: [libc++] Assume that builtins for math.h functions are available

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF requested changes to this revision.
EricWF added a comment.

In D135781#3894426 <https://reviews.llvm.org/D135781#3894426>, @philnik wrote:

> 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.

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.



================
Comment at: libcxx/include/math.h:850
+  using __result_type = typename std::__promote<_Tp, _Up>::type;
+  return ::copysign(static_cast<__result_type>(__x), static_cast<__result_type>(__y));
 }
----------------
ldionne wrote:
> 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);
> }
> ```
Good spotting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135781/new/

https://reviews.llvm.org/D135781