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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/math.h:1005
 _LIBCPP_CONSTEXPR
-#endif
 inline _LIBCPP_HIDE_FROM_ABI long double __libcpp_copysign(long double __x, long double __y) _NOEXCEPT {
   return __builtin_copysignl(__x, __y);
----------------
Can you make a TODO to clean this up?


================
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));
 }
----------------
EricWF wrote:
> 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.
@philnik Can you please triple check that this is not an issue anymore? I'm approving this but contingent on this not being an issue.

This behavior would be really, really not acceptable for users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135781