Improve support for std::optional-like classes #850
Conversation
With older versions of Boost, `value = {}` can fail to compile because
the interpretation of `{}` is ambiguous (could construct `boost::none`
or the value type). It can also fail with `std::experimental::optional`,
at least on GCC 5.4, because it would construct an instance of the
optional type and then move-assign it, and if the value type isn't move
assignable this would fail.
This is replaced by preferring `.reset` if the type supports it, and
also providing a traits class that can be extended to override the
behaviour where necessary (which is done for
std::experimental::optional).
Additionally, the assignment of the value from the inner caster was by
copy rather than move, which prevented use with uncopyable types.
Closes pybind#847.
include/pybind11/stl.h
Outdated
| return true; | ||
| } | ||
| value_conv inner_caster; | ||
| if (!inner_caster.load(src, convert)) | ||
| return false; | ||
|
|
||
| value.emplace(cast_op<typename T::value_type>(inner_caster)); | ||
| value.emplace(std::move(cast_op<typename T::value_type>(inner_caster))); |
There was a problem hiding this comment.
Can someone sanity-check me that this is safe? I don't have an understanding of how all the casting magic works yet.
There was a problem hiding this comment.
Not safe: type casters can return a lvalue reference to a variable they don't own. We might, however, be able to coax type casters cast ops in rvalue context into giving up an rvalue reference.
It was moving the output of a caster, which is not safe because a caster can return an lvalue reference which it doesn't own. The wrapped type must now be copy-constructible, but does not need to be either move- or copy-assignable.
|
Does the If it does, is it possible/would it be better to SFINAE on |
|
I haven't seen any deprecated warnings from boost::optional, at least with version 1.58. My main reason for making the |
|
I've also submitted https://svn.boost.org/trac/boost/ticket/13031 to suggest un-deprecating reset. |
|
AFAIK casters are generally only used once, except for some subcasters for vectors, maps, etc. But it looks like #851 makes those single-use-only casters as well. So if all caster are only ever used once, a possible simple solution would be to remove |
|
You might also report via https://github.com/boostorg/optional/issues |
| // both exist then the .reset version is preferred due to a better match | ||
| // in the second argument. | ||
| template<typename T2 = T> | ||
| static decltype(std::declval<T2>().reset()) reset_impl(T &value, int) { |
There was a problem hiding this comment.
static void_t<decltype(std::declval<T2>().reset())> reset_impl... would be better here.
There was a problem hiding this comment.
And return value.reset(); -> value.reset();
| } | ||
|
|
||
| template<typename T2 = T> | ||
| static decltype(std::declval<T2>() = {}) reset_impl(T &value, long) { |
|
Any thoughts on dispensing with if (src.is_none()) {
value = {}; // nullopt
return true;
}with: if (src.is_none()) {
return true; // default constructed `value` is already `nullopt`
} |
|
I think, with #851 now merged, there isn't any multiple- |
It would certainly reduce complexity, if you're sure that all casters are single-use. |
|
Closed in favor of #874. |
With older versions of Boost,
value = {}can fail to compile becausethe interpretation of
{}is ambiguous (could constructboost::noneor the value type). It can also fail with
std::experimental::optional,at least on GCC 5.4, because it would construct an instance of the
optional type and then move-assign it, and if the value type isn't move
assignable this would fail.
This is replaced by preferring
.resetif the type supports it, andalso providing a traits class that can be extended to override the
behaviour where necessary (which is done for
std::experimental::optional).
Additionally, the assignment of the value from the inner caster was by
copy rather than move, which prevented use with uncopyable types.
Closes #847.