Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++] Signature of complex pow does not allow some user overloads #109858

Closed
SteveBronder opened this issue Sep 24, 2024 · 5 comments · Fixed by #110235
Closed

[libc++] Signature of complex pow does not allow some user overloads #109858

SteveBronder opened this issue Sep 24, 2024 · 5 comments · Fixed by #110235
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Comments

@SteveBronder
Copy link

Testing the Stan math library against clang 19.0.1 we found that one of libc++'s complex pow signatures is overriding one of our signatures. stan-dev/math#3106

We can fix this by making the exponent's type const&, or by users calling stan::math::pow fully instead of bringing pow into the local scope. But I wanted to suggest a fix here that I think will stop this issue from coming up for other users.

In the pow signatures for a complex base and arithmetic exponent, such as the one below from complex, if a user has a custom type in complex this signature will be chosen by the compiler but then fail while calling __promote

template <class _Tp, class _Up, __enable_if_t<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));
}

This signature only checks that Up_ is arithmetic while the compiler is looking for candidates, but then after that signature has been chosen it also requires that _Tp is arithmetic since __promote<_Tp, _Up> will throw a compiler error from it's static assert if both types are not arithmetic (code). It looks like the signature should be checking that __promote<_Tp, _Up> is arithmetic so that users with custom types can add their own pow functions with custom complex types.

This could be fixed by adding another NTTP to these functions that checks if the result of __promote is valid.

template <class _Tp, class _Up, 
  __enable_if_t<is_arithmetic<_Up>::value, int> = 0,
  __enable_if_t<is_arithmetic_promotable<_Tp, _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));
}

The example of the fix and issue can be found in the godbolt below. You can toggle the SHOW_ERROR macro on and off to see the current error and suggested fix

https://godbolt.org/z/1vE665xqT

@github-actions github-actions bot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 24, 2024
@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Sep 25, 2024

Per N4868 [cmplx.over]/3, I think we should implement the additional overloads like the following in C++20 and earlier modes (i.e. constrain them harder).

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_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 && 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));
}
Some crux in C++23

But the standard requirements was changed in C++23 by P1467R9, and IIUC that the type of std::pow(1, std::complex<float>(1.0f)) was changed from std::complex<double> to std::complex<float>. I guess this aspect of changes for pow is defective and have tried to submit an LWG issue.

For standard floating-point types (float, double, and long double) and corresponding complex types, I don't think the behavior should change in C++23.

@SteveBronder
Copy link
Author

Thanks @frederick-vs-ja I think the overloads you have make sense. Happy to make a PR for this.

@frederick-vs-ja
Copy link
Contributor

Not sure whether the "fix" would be preferred because the standard says that instantiating std::complex<NonFloatingPoint> has unspecified effects...

@SteveBronder
Copy link
Author

From cppreference I'm seeing

The behavior is unspecified (and may fail to compile) if T is not a cv-unqualified standard(until C++23) floating-point type

Which is fine because we specify what would happen for our type :)

@SteveBronder
Copy link
Author

Thank you!

NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this issue Nov 4, 2024
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants