llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.04k stars 11.98k forks source link

std::complex with a custom type does not work because of how std::__promote is defined #29937

Closed hfinkel closed 5 years ago

hfinkel commented 8 years ago
Bugzilla Link 30589
Resolution INVALID
Resolved on Jun 03, 2019 18:48
Version unspecified
OS Linux
Blocks llvm/llvm-project#19563
Attachments test case
CC @mclow

Extended Description

In https://reviews.llvm.org/D18639, Eric asked if we supporting using std::complex with custom numeric types. The answer is: mostly. I've attached the test case I started writing. This compiles with libstdc++, and in fact, requires a lot less of the custom type than we do (e.g. we require fabs in addition to abs, scalbn, logb, and many other functions). With libc++, however, we do it one compilation error which looks like a problem with libc++ itself:

include/c++/v1/complex:1321:39: error: no matching function for call to 'pow' complex<_Tp> z = log(x + sqrt(pow(x, _Tp(2)) - _Tp(1))); ... include/c++/v1/complex:1109:1: note: candidate template ignored: substitution failure [with _Tp = ct::va lue, _Up = ct::value]: no type named 'type' in 'std::1::promote<ct::value, ct::value, void>' pow(const complex<_Tp>& __x, const _Up& y)

The problem here seems to be that std::__promote, which is defined in type_traits, only works for types for which __numeric_type::type exists, and that is only for builtin numeric types. As a result, this template does not match:

template<class _Tp, class _Up> inline _LIBCPP_INLINE_VISIBILITY typename enable_if < is_arithmetic<_Up>::value, complex<typename __promote<_Tp, _Up>::type>

::type pow(const complex<_Tp>& x, const _Up& y) { typedef complex<typename promote<_Tp, _Up>::type> result_type; return _VSTD::pow(result_type(x), result_type(__y)); }

mclow commented 5 years ago

I'm going to close this bug. We're compliant here - the results are "unspecified".

If we want to add support for custom types to our complex implementation, that's fine. But this bug isn't going to help is.

mclow commented 7 years ago

I'm leaning towards option #​2 - basically, static_assert.

I think that's better than "sorta working" or "working for some things and not others".

The other choice would be your option #​5 - make everything work - and for that I would be happy to review patches.

hfinkel commented 8 years ago

Do you happen to know if there's any history here of the committee discussing specifically allowing other types?

I'm sure there was such a discussion; otherwise the paragraph that I quoted would not be there in the standard.

However, the first sentence of that paragraph has been unchanged in the standard since 1998; so any such discussion was a long time ago.

Indeed ;)

In any case, any opinion on how to move forward. All things considered, I think that my option 4 is probably the best, and I'd like to propose updating the standard to allow the same. There are good use cases for this, and I expect that all, or almost all, implementations already work in this regard.

FWIW, boost_1_62_0/libs/numeric/ublas/benchmarks/bench4/bench4.cpp has:

74 #ifdef USE_BOOST_COMPLEX 75 76 template struct peak<boost::complex<boost::numeric::interval > >; 77 template struct peak<boost::complex<boost::numeric::interval > >; 78 79 #endif

which does represent a good use case.

mclow commented 8 years ago

Do you happen to know if there's any history here of the committee discussing specifically allowing other types?

I'm sure there was such a discussion; otherwise the paragraph that I quoted would not be there in the standard.

However, the first sentence of that paragraph has been unchanged in the standard since 1998; so any such discussion was a long time ago.

hfinkel commented 8 years ago

This also brings up another issue: Does the standard say anything about what is required of a type T for std::complex to work.

[complex.numbers]/2 says: The effect of instantiating the template complex for any type other than float, double, or long double is unspecified. The specializations complex, complex, and complex are literal types (3.9).

What do you think we should do; there seem to be many options:

  1. Do nothing.
  2. Force the instantiation to fail for other types. Add a test for that.
  3. Have the compiler issue a warning if we instantiate with other types (but otherwise do nothing).
  4. Add a test for instantiation using a custom type, but test only basic operations (saying we support that as an extension, but not the transcendental functions).
  5. Fix things so everything, including the transcendental functions, work on custom types, and then add a test covering everything.

Do you happen to know if there's any history here of the committee discussing specifically allowing other types?

mclow commented 8 years ago

This also brings up another issue: Does the standard say anything about what is required of a type T for std::complex to work.

[complex.numbers]/2 says: The effect of instantiating the template complex for any type other than float, double, or long double is unspecified. The specializations complex, complex, and complex are literal types (3.9).

hfinkel commented 8 years ago

This also brings up another issue: Does the standard say anything about what is required of a type T for std::complex to work (i.e. for the various functions defined in to do something)?