[libc++] Constrain additional overloads of `pow` for `complex` harder #110235

frederick-vs-ja · 2024-09-27T10:12:06Z

The changes in #81379 broke some 3rd party library code that expected usability of std::complex<NonFloatingPoint>. Although such code isn't portable per [complex.numbers.general]/2, it might be better to make these additional overloads not to interfere overload resolution too much.

llvmbot · 2024-09-27T10:12:43Z

@llvm/pr-subscribers-libcxx

Author: A. Jiang (frederick-vs-ja)

Changes

Fixes #109858.

The changes in #81379 broke some 3rd party library code that expected usability of std::complex<NonFloatingPoint>. Although such code isn't portable per [complex.numbers.general]/2, it might be better to make these additional overloads not to interfere overload resolution too much.

Full diff: https://github.com/llvm/llvm-project/pull/110235.diff

2 Files Affected:

(modified) libcxx/include/complex (+3-3)
(added) libcxx/test/libcxx/numerics/complex.number/cmplx.over.pow.pass.cpp (+106)

diff --git a/libcxx/include/complex b/libcxx/include/complex
index 4030d96b003d56..15e42800fbfa0a 100644
--- a/libcxx/include/complex
+++ b/libcxx/include/complex
@@ -1097,20 +1097,20 @@ inline _LIBCPP_HIDE_FROM_ABI complex<_Tp> pow(const complex<_Tp>& __x, const com
   return std::exp(__y * std::log(__x));
 }
 
-template <class _Tp, class _Up>
+template <class _Tp, class _Up, __enable_if_t<is_floating_point<_Tp>::value && is_floating_point<_Up>::value, int> = 0>
 inline _LIBCPP_HIDE_FROM_ABI complex<typename __promote<_Tp, _Up>::type>
 pow(const complex<_Tp>& __x, const complex<_Up>& __y) {
   typedef complex<typename __promote<_Tp, _Up>::type> result_type;
   return std::pow(result_type(__x), result_type(__y));
 }
 
-template <class _Tp, class _Up, __enable_if_t<is_arithmetic<_Up>::value, int> = 0>
+template <class _Tp, class _Up, __enable_if_t<is_floating_point<_Tp>::value && is_arithmetic<_Up>::value, int> = 0>
 inline _LIBCPP_HIDE_FROM_ABI complex<typename __promote<_Tp, _Up>::type> pow(const complex<_Tp>& __x, const _Up& __y) {
   typedef complex<typename __promote<_Tp, _Up>::type> result_type;
   return std::pow(result_type(__x), result_type(__y));
 }
 
-template <class _Tp, class _Up, __enable_if_t<is_arithmetic<_Tp>::value, int> = 0>
+template <class _Tp, class _Up, __enable_if_t<is_arithmetic<_Tp>::value && is_floating_point<_Up>::value, int> = 0>
 inline _LIBCPP_HIDE_FROM_ABI complex<typename __promote<_Tp, _Up>::type> pow(const _Tp& __x, const complex<_Up>& __y) {
   typedef complex<typename __promote<_Tp, _Up>::type> result_type;
   return std::pow(result_type(__x), result_type(__y));
diff --git a/libcxx/test/libcxx/numerics/complex.number/cmplx.over.pow.pass.cpp b/libcxx/test/libcxx/numerics/complex.number/cmplx.over.pow.pass.cpp
new file mode 100644
index 00000000000000..64e679fed7435c
--- /dev/null
+++ b/libcxx/test/libcxx/numerics/complex.number/cmplx.over.pow.pass.cpp
@@ -0,0 +1,106 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <complex>
+
+//  template<class T, class U> complex<__promote<T, U>::type> pow(const complex<T>&, const U&);
+//  template<class T, class U> complex<__promote<T, U>::type> pow(const complex<T>&, const complex<U>&);
+//  template<class T, class U> complex<__promote<T, U>::type> pow(const T&, const complex<U>&);
+
+// Test that these additional overloads are free from catching std::complex<non-floating-point>,
+// which is expected by several 3rd party libraries, see https://github.com/llvm/llvm-project/issues/109858.
+
+#include <cassert>
+#include <cmath>
+#include <complex>
+#include <type_traits>
+
+#include "test_macros.h"
+
+namespace usr {
+struct usr_tag {};
+
+template <class T, class U>
+TEST_CONSTEXPR
+    typename std::enable_if<(std::is_same<T, usr_tag>::value && std::is_floating_point<U>::value) ||
+                                (std::is_floating_point<T>::value && std::is_same<U, usr_tag>::value),
+                            int>::type
+    pow(const T&, const std::complex<U>&) {
+  return std::is_same<T, usr_tag>::value ? 0 : 1;
+}
+
+template <class T, class U>
+TEST_CONSTEXPR
+    typename std::enable_if<(std::is_same<T, usr_tag>::value && std::is_floating_point<U>::value) ||
+                                (std::is_floating_point<T>::value && std::is_same<U, usr_tag>::value),
+                            int>::type
+    pow(const std::complex<T>&, const U&) {
+  return std::is_same<U, usr_tag>::value ? 2 : 3;
+}
+
+template <class T, class U>
+TEST_CONSTEXPR
+    typename std::enable_if<(std::is_same<T, usr_tag>::value && std::is_floating_point<U>::value) ||
+                                (std::is_floating_point<T>::value && std::is_same<U, usr_tag>::value),
+                            int>::type
+    pow(const std::complex<T>&, const std::complex<U>&) {
+  return std::is_same<T, usr_tag>::value ? 4 : 5;
+}
+} // namespace usr
+
+int main(int, char**) {
+  using std::pow;
+  using usr::pow;
+
+  TEST_CONSTEXPR usr::usr_tag tag;
+  TEST_CONSTEXPR_CXX14 const std::complex<usr::usr_tag> ctag;
+
+  assert(pow(tag, std::complex<float>(1.0f)) == 0);
+  assert(pow(std::complex<float>(1.0f), tag) == 2);
+  assert(pow(tag, std::complex<double>(1.0)) == 0);
+  assert(pow(std::complex<double>(1.0), tag) == 2);
+  assert(pow(tag, std::complex<long double>(1.0l)) == 0);
+  assert(pow(std::complex<long double>(1.0l), tag) == 2);
+
+  assert(pow(1.0f, ctag) == 1);
+  assert(pow(ctag, 1.0f) == 3);
+  assert(pow(1.0, ctag) == 1);
+  assert(pow(ctag, 1.0) == 3);
+  assert(pow(1.0l, ctag) == 1);
+  assert(pow(ctag, 1.0l) == 3);
+
+  assert(pow(ctag, std::complex<float>(1.0f)) == 4);
+  assert(pow(std::complex<float>(1.0f), ctag) == 5);
+  assert(pow(ctag, std::complex<double>(1.0)) == 4);
+  assert(pow(std::complex<double>(1.0), ctag) == 5);
+  assert(pow(ctag, std::complex<long double>(1.0l)) == 4);
+  assert(pow(std::complex<long double>(1.0l), ctag) == 5);
+
+#if TEST_STD_VER >= 11
+  static_assert(pow(tag, std::complex<float>(1.0f)) == 0, "");
+  static_assert(pow(std::complex<float>(1.0f), tag) == 2, "");
+  static_assert(pow(tag, std::complex<double>(1.0)) == 0, "");
+  static_assert(pow(std::complex<double>(1.0), tag) == 2, "");
+  static_assert(pow(tag, std::complex<long double>(1.0l)) == 0, "");
+  static_assert(pow(std::complex<long double>(1.0l), tag) == 2, "");
+
+  static_assert(pow(1.0f, ctag) == 1, "");
+  static_assert(pow(ctag, 1.0f) == 3, "");
+  static_assert(pow(1.0, ctag) == 1, "");
+  static_assert(pow(ctag, 1.0) == 3, "");
+  static_assert(pow(1.0l, ctag) == 1, "");
+  static_assert(pow(ctag, 1.0l) == 3, "");
+
+  static_assert(pow(ctag, std::complex<float>(1.0f)) == 4, "");
+  static_assert(pow(std::complex<float>(1.0f), ctag) == 5, "");
+  static_assert(pow(ctag, std::complex<double>(1.0)) == 4, "");
+  static_assert(pow(std::complex<double>(1.0), ctag) == 5, "");
+  static_assert(pow(ctag, std::complex<long double>(1.0l)) == 4, "");
+  static_assert(pow(std::complex<long double>(1.0l), ctag) == 5, "");
+#endif
+}

ldionne · 2024-10-01T21:55:31Z

libcxx/include/complex

 inline _LIBCPP_HIDE_FROM_ABI complex<typename __promote<_Tp, _Up>::type> pow(const complex<_Tp>& __x, const _Up& __y) {
  typedef complex<typename __promote<_Tp, _Up>::type> result_type;
  return std::pow(result_type(__x), result_type(__y));
 }

-template <class _Tp, class _Up, __enable_if_t<is_arithmetic<_Tp>::value, int> = 0>


Is there a reason why this isn't an issue for e.g. std::asinh(std::complex<T>) defined below? I'm wary of trying to work around something that is explicitly unspecified in the Standard.

The original issue seemed caused by recent changes of __promote, and in <complex> only additional overloads of pow use __promote.
#81379 made __promote reject non-arithmetic types in an SFINAE-unfriendly way, and previously __promote accidently accepted some program-defined non-arithmetic types as long as their operator+'s work as expected.

ldionne

I am really ambivalent about this change. On the one hand it's a small change and it avoids an immediate breakage in the wild. On the other hand, it also seems to promise something that I'm not certain we want to (or will be able to) promise in the future, i.e. that libc++ std::complex works with non standard floating point types.

@philnik777 I'd like to have your opinion on this.

libcxx/test/libcxx/numerics/complex.number/cmplx.over.pow.pass.cpp

- `return 0;` for freestanding environments. - drop constexpr coverage

philnik777 · 2024-10-21T18:30:07Z

Personally I don't think we want to guarantee this. I'm also not sure whether this would break other code in the wild that uses e.g. complex<int> (not that that's any more sanctioned). FWIW I'd be more in favour of simply removing the static_assert again, since I've only added it because I thought that was a requirement before my change. But I wouldn't want to guarantee that we don't break users this way at a later point again.

frederick-vs-ja · 2024-10-22T06:59:24Z

FWIW I'd be more in favour of simply removing the static_assert again, since I've only added it because I thought that was a requirement before my change.

Hmm... Just removing static_assert doesn't make __promote SFINAE-friendly, and will make the error messages worse for the case in #109858.

In libc++, we generally constrain the additional overloads (in <cmath>) that use __promote to accept only arithmetic types. But currently we are not doing so for pow overloads for complex, so hard error can raise from them.

philnik777 · 2024-10-22T09:08:36Z

FWIW I'd be more in favour of simply removing the static_assert again, since I've only added it because I thought that was a requirement before my change.

Hmm... Just removing static_assert doesn't make __promote SFINAE-friendly, and will make the error messages worse for the case in #109858.

In libc++, we generally constrain the additional overloads (in <cmath>) that use __promote to accept only arithmetic types. But currently we are not doing so for pow overloads for complex, so hard error can raise from them.

Well, the overloads for complex are constrained to only accepting complex. The only way to trigger this is to specialize complex, which is already kinda fishy IMO.

ldionne · 2024-10-22T12:57:06Z

@frederick-vs-ja Do you know what other implementations promise w.r.t. std::complex<UserDefined>? Does it work with other implementations? If so, perhaps it would be worth changing the spec to require that it does.

If not, I think the consensus is leaning towards not supporting this, and closing #109858 as not-to-be-fixed.

frederick-vs-ja · 2024-10-25T09:57:16Z

@frederick-vs-ja Do you know what other implementations promise w.r.t. std::complex<UserDefined>? Does it work with other implementations? If so, perhaps it would be worth changing the spec to require that it does.

I don't think there's any implementation promising this. But the example works with libstdc++ because its calculation for promoted FP type is SFINAE-friendly...

ldionne

Let's do this. Let's punt on deciding whether we should promise this extension, but for the time being, let's fix the code.

libcxx/test/libcxx/numerics/complex.number/cmplx.over.pow.pass.cpp

Co-authored-by: Louis Dionne <[email protected]>

…llvm#110235) Fixes llvm#109858. The changes in llvm#81379 broke some 3rd party library code that expected usability of `std::complex<NonFloatingPoint>`. Although such code isn't portable per [complex.numbers.general]/2, it might be better to make these additional overloads not to interfere overload resolution too much. --------- Co-authored-by: Louis Dionne <[email protected]>

[libc++] Constrain additional overloads of pow for complex harder

b3c2eea

frederick-vs-ja requested a review from philnik777 September 27, 2024 10:12

frederick-vs-ja requested a review from a team as a code owner September 27, 2024 10:12

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 27, 2024

frederick-vs-ja mentioned this pull request Sep 27, 2024

[libc++] Are some standard overloads of std::pow missing? #110241

Closed

ldionne reviewed Oct 1, 2024

View reviewed changes

ldionne reviewed Oct 15, 2024

View reviewed changes

libcxx/test/libcxx/numerics/complex.number/cmplx.over.pow.pass.cpp Outdated Show resolved Hide resolved

libcxx/test/libcxx/numerics/complex.number/cmplx.over.pow.pass.cpp Outdated Show resolved Hide resolved

frederick-vs-ja added 2 commits October 19, 2024 22:38

Address @ldionne's review comments

84e7c7b

- `return 0;` for freestanding environments. - drop constexpr coverage

Merge branch 'main' into complex-pow-constraints

602372d

ldionne approved these changes Oct 25, 2024

View reviewed changes

libcxx/test/libcxx/numerics/complex.number/cmplx.over.pow.pass.cpp Outdated Show resolved Hide resolved

Adopt @ldionne's clarification for comments

f6b91cf

Co-authored-by: Louis Dionne <[email protected]>

ldionne approved these changes Oct 29, 2024

View reviewed changes

frederick-vs-ja merged commit 0f8dbb2 into llvm:main Oct 29, 2024
62 checks passed

frederick-vs-ja deleted the complex-pow-constraints branch October 29, 2024 23:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Constrain additional overloads of `pow` for `complex` harder #110235

[libc++] Constrain additional overloads of `pow` for `complex` harder #110235

frederick-vs-ja commented Sep 27, 2024

llvmbot commented Sep 27, 2024

ldionne Oct 1, 2024

frederick-vs-ja Oct 14, 2024

ldionne left a comment

philnik777 commented Oct 21, 2024

frederick-vs-ja commented Oct 22, 2024

philnik777 commented Oct 22, 2024

ldionne commented Oct 22, 2024

frederick-vs-ja commented Oct 25, 2024

ldionne left a comment

[libc++] Constrain additional overloads of pow for complex harder #110235

[libc++] Constrain additional overloads of pow for complex harder #110235

Conversation

frederick-vs-ja commented Sep 27, 2024

llvmbot commented Sep 27, 2024

ldionne Oct 1, 2024

Choose a reason for hiding this comment

frederick-vs-ja Oct 14, 2024

Choose a reason for hiding this comment

ldionne left a comment

Choose a reason for hiding this comment

philnik777 commented Oct 21, 2024

frederick-vs-ja commented Oct 22, 2024

philnik777 commented Oct 22, 2024

ldionne commented Oct 22, 2024

frederick-vs-ja commented Oct 25, 2024

ldionne left a comment

Choose a reason for hiding this comment

[libc++] Constrain additional overloads of `pow` for `complex` harder #110235

[libc++] Constrain additional overloads of `pow` for `complex` harder #110235