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

Jump to bottom

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

Closed
hfinkel opened this issue Oct 1, 2016 · 7 comments
Closed

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

hfinkel opened this issue Oct 1, 2016 · 7 comments
Labels
bugzilla Issues migrated from bugzilla invalid Resolved as invalid, i.e. not a bug libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Comments

@hfinkel
Copy link
Collaborator

hfinkel commented Oct 1, 2016

Bugzilla Link 30589
Resolution INVALID
Resolved on Jun 03, 2019 18:48
Version unspecified
OS Linux
Blocks #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));
}
@hfinkel
Copy link
Collaborator Author

hfinkel commented Oct 1, 2016

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)?

@mclow
Copy link
Contributor

mclow commented Oct 2, 2016

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
Copy link
Collaborator Author

hfinkel commented Oct 2, 2016

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
Copy link
Contributor

mclow commented Oct 2, 2016

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
Copy link
Collaborator Author

hfinkel commented Oct 3, 2016

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
Copy link
Contributor

mclow commented May 18, 2017

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.

@mclow
Copy link
Contributor

mclow commented Jun 4, 2019

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.

@llvmbot llvmbot mentioned this issue Mar 19, 2014
Open
@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bugzilla Issues migrated from bugzilla invalid Resolved as invalid, i.e. not a bug libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mclow @hfinkel