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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I'm not a big fan of the duplication, but unless we find a way to improve that, I think this LGTM (I mean.. I don't see a better way right now). Requesting changes until we've resolved the discussion about duplication.

Thanks for working on this, I think it'll make the constexpr-for-complex patch better.



================
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:
> 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?


================
Comment at: libcxx/include/math.h:1171
+copysign(float __lcpp_x, float __lcpp_y) _NOEXCEPT {
+  return __copysign(__lcpp_x, __lcpp_y);
+}
----------------
`_VSTD::` here and below (it doesn't make a difference here, but let's be consistent).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106364