Untangle cast logic to not implicitly require castability #1442
Conversation
include/pybind11/cast.h
Outdated
| struct is_explicitly_convertible | ||
| { | ||
| enum {value = std::is_same<A, B>::value || (std::is_constructible<B, A>::value && !std::is_convertible<A, B>::value)}; | ||
| }; |
There was a problem hiding this comment.
This struct can be simplified considerably:
template <class A, class B> using is_explicitly_convertible = bool_constant<
std::is_same<A, B>::value || (std::is_constructible<B, A>::value && !std::is_convertible<A, B>::value>;Edit: But I'm curious why the not-implicitly-convertible check is needed here. Wouldn't it be simpler to just use is_constructible, i.e.:
template <class A, class B> using is_explicitly_convertible = any_of<std::is_same<A, B>, std::is_constructible<B, A>>;There was a problem hiding this comment.
std::bool_constant is a relatively new addition in C++17 and would require a break. Nevertheless, it is a great suggestion and really valuable feedback. I went ahead and used std::integral_constant<bool, ..> instead in 416a43f.
There was a problem hiding this comment.
But I'm curious why the not-implicitly-convertible check is needed here.
It makes sure the overload doesn't become ambiguous.
include/pybind11/cast.h
Outdated
| template<typename U = T> | ||
| static typename std::enable_if<!std::is_floating_point<U>::value && is_explicitly_convertible<U, unsigned long long>::value, handle>::type | ||
| cast(U src, return_value_policy /* policy */, handle /* parent */) { | ||
| return PyLong_FromUnsignedLongLong((unsigned long long) src); | ||
| } |
There was a problem hiding this comment.
Splitting this up into an exhaustive list of types doesn't strike me as a good idea: aside from being easy to miss some in the standard (for example, you're missing signed char, char16_t, char32_t, wchar_t), it also excludes custom types that a compiler might support or a third-party library might add is_integral specializations for.
I'm also having a bit of trouble seeing what this accomplishes (a test case to demonstrate the current failure would help). The referenced type-safe-handles type doesn't seem immediately applicable to me as it doesn't seem like it will be caught in this code at all (this is already inside a std::is_arithemetic type caster specialization).
There was a problem hiding this comment.
Splitting this up into an exhaustive list of types doesn't strike me as a good idea: aside from being easy to miss some in the standard (for example, you're missing signed char, char16_t, char32_t, wchar_t), it also excludes custom types that a compiler might support or a third-party library might add is_integral specializations for.
Agree, an exhaustive list is not beautiful at all and perhaps it is not complete. Support for signed char should be there, though.
I'm also having a bit of trouble seeing what this accomplishes (a test case to demonstrate the current failure would help). The referenced type-safe-handles type doesn't seem immediately applicable to me as it doesn't seem like it will be caught in this code at all (this is already inside a std::is_arithemetic type caster specialization).
The current code in master branch requires any type to be cast-able to floating point as well as to castable to an integral type. Otherwise the code is malformed and won't compile. Arguably this patch can be improved, but it uses a SFINAE-like approach to make sure types don't have to cast-able to fp and int numbers.
|
@jagerman Please have a look at the latest revision. It should simplify things a lot. |
|
friendly ping |
|
The change looks good to me. I'll merge tomorrow if no one else has any comments. |
|
cool, thanks @jagerman |
|
friendly ping |
|
Oops, sorry, got tired up with other things and forgot about this. Merged; thanks for the PR! |
…stability (pybind#1442)" This reverts commit 221fb1e.
The current code requires implicitly that integral types are cast-able to floating point. In case of strongly-typed integrals (e.g. as explained here) this is not always the case.
The patch at hand uses compile-time magic to make sure that cast-ability is only requested if the type supports it.