[libcxx-commits] [PATCH] D106364: [libc++] Add __copysign conditionally constexpr overloads.

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added inline comments.


================
Comment at: libcxx/include/math.h:1169-1190
+inline _LIBCPP_INLINE_VISIBILITY float
+copysign(float __lcpp_x, float __lcpp_y) _NOEXCEPT {
+  return __copysign(__lcpp_x, __lcpp_y);
+}
+
+inline _LIBCPP_INLINE_VISIBILITY long double
+copysign(long double __lcpp_x, long double __lcpp_y) _NOEXCEPT {
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > curdeius wrote:
> > > > If anyone has an idea about how to replace these 3 overloads with something simpler, without breaking things, I'm all ears.
> > > > Just writing:
> > > > ```
> > > > template <class _A1, class _A2>
> > > > auto copysign(_A1 __lcpp_x, _A2 __lcpp_y) _NOEXCEPT {
> > > >     return __copysign(__lcpp_x, __lcpp_y);
> > > > }
> > > > ```
> > > > doesn't work before C++14 because of `auto`.
> > > > Using `std::__promote` instead results in ambiguities.
> > > > Also, type deduction might fail if passed e.g. a float and a type implicitly convertible to float (works now, deduction of _A2 would fail without using tricks like `_A2 = identity<_A1>`, but even then I'm not sure of consequences.
> > > At least it needs more comments about the design constraints ;) because I don't see why there are overloads only for `float` and `long double`, but not `double`. What's wrong with simply...?
> > > ```
> > > #if __has_builtin(__builtin_copysignf)
> > > inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR float
> > > copysign(float __x, float __y) _NOEXCEPT
> > >     { return __builtin_copysignf(__x, __y); }
> > > #else
> > > inline _LIBCPP_INLINE_VISIBILITY float
> > > copysign(float __x, float __y) _NOEXCEPT
> > >     { return ::copysignf(__x, __y); }
> > > #endif
> > > 
> > > #if __has_builtin(__builtin_copysign)
> > > inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR double
> > > copysign(double __x, double __y) _NOEXCEPT
> > >     { return __builtin_copysign(__x, __y); }
> > > #else
> > > inline _LIBCPP_INLINE_VISIBILITY double
> > > copysign(double __x, double __y) _NOEXCEPT
> > >     { return ::copysign(__x, __y); }
> > > #endif
> > > 
> > > #if __has_builtin(__builtin_copysignl)
> > > inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR long double
> > > copysign(long double __x, long double __y) _NOEXCEPT
> > >     { return __builtin_copysignl(__x, __y); }
> > > #else
> > > inline _LIBCPP_INLINE_VISIBILITY long double
> > > copysign(long double __x, long double __y) _NOEXCEPT
> > >     { return ::copysignl(__x, __y); }
> > > #endif
> > > 
> > > inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
> > > typename _EnableIf<
> > >     is_arithmetic<_A1>::value && is_arithmetic<_A2>::value,
> > >     __promote<_A1, _A2>
> > > >::type
> > > copysign(_A1 __x, _A2 __y) _NOEXCEPT
> > > {
> > >     typedef typename std::__promote<_A1, _A2>::type __result_type;
> > >     static_assert((!(std::_IsSame<_A1, __result_type>::value &&
> > >                      std::_IsSame<_A2, __result_type>::value)), "");
> > >     return _VSTD::copysign(__result_type(__x), __result_type(__y));
> > > }
> > > ```
> > > It's about 6 lines longer than what's there now, but I think it's massively simpler.
> > We don't want to make `copysign` `constexpr`, only the library-internal version `__copysign`.
> > 
> > Regarding an overload for `double`, it is handled by the `_A1, _A2` overload which ends up calling `::copysign(double, double)` from the C library. Or were you thinking about something else?
> > We don't want to make `copysign` constexpr, only the library-internal version `__copysign`.
> Oh, I see.  In that case, my suggestion would make the code ~2x as long as it was before; and so I have no suggestion better than this PR.
> 
> > Regarding an overload for `double`, it is handled by the `_A1, _A2` overload
> My godbolt testing suggests that `copysign(double, double)` is //not// handled by the `_A1, _A2` overload, but instead by `using ::copysign _LIBCPP_USING_IF_EXISTS;` elsewhere in this file... Ah, but `__copysign(double, double)` //is// handled by the template. Which is fine (only) because users don't call `__copysign` directly. But actually, does //libc++// ever call `__copysign(_A1, _A2)` directly? Maybe the templated version should be provided only for users, i.e. template `copysign` because the Standard says we have to, but do not template `__copysign`.
> We don't want to make `copysign` `constexpr`, only the library-internal version `__copysign`.
Actually, why don't we?
Can it break anything?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106364