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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM pending CI.



================
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 {
----------------
curdeius wrote:
> 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?
1. If there ever was a reason why `copysign` couldn't be constexpr anymore, it would be API breaking for us to make that change. It's arguably not going to happen for something like `copysign`, though, but I think it's better to be consistent everywhere and not add `constexpr` as an extension.

2. It's pedantic, but the standard does not allow us to add `constexpr` as an extension.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106364