Skip to content

Commit 617fbcf

Browse files
jagermanwjakob
authored andcommitted
Fix stl_bind to support movable, non-copyable value types (#490)
This commit includes the following changes: * Don't provide make_copy_constructor for non-copyable container make_copy_constructor currently fails for various stl containers (e.g. std::vector, std::unordered_map, std::deque, etc.) when the container's value type (e.g. the "T" or the std::pair<K,T> for a map) is non-copyable. This adds an override that, for types that look like containers, also requires that the value_type be copyable. * stl_bind.h: make bind_{vector,map} work for non-copy-constructible types Most stl_bind modifiers require copying, so if the type isn't copy constructible, we provide a read-only interface instead. In practice, this means that if the type is non-copyable, it will be, for all intents and purposes, read-only from the Python side (but currently it simply fails to compile with such a container). It is still possible for the caller to provide an interface manually (by defining methods on the returned class_ object), but this isn't something stl_bind can handle because the C++ code to construct values is going to be highly dependent on the container value_type. * stl_bind: copy only for arithmetic value types For non-primitive types, we may well be copying some complex type, when returning by reference is more appropriate. This commit returns by internal reference for all but basic arithmetic types. * Return by reference whenever possible Only if we definitely can't--i.e. std::vector<bool>--because v[i] returns something that isn't a T& do we copy; for everything else, we return by reference. For the map case, we can always return by reference (at least for the default stl map/unordered_map).
1 parent 06bd27f commit 617fbcf

File tree

4 files changed

+293
-110
lines changed

4 files changed

+293
-110
lines changed

include/pybind11/cast.h

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,18 @@ using cast_op_type = typename std::conditional<std::is_pointer<typename std::rem
346346
typename std::add_pointer<intrinsic_t<T>>::type,
347347
typename std::add_lvalue_reference<intrinsic_t<T>>::type>::type;
348348

349+
// std::is_copy_constructible isn't quite enough: it lets std::vector<T> (and similar) through when
350+
// T is non-copyable, but code containing such a copy constructor fails to actually compile.
351+
template <typename T, typename SFINAE = void> struct is_copy_constructible : std::is_copy_constructible<T> {};
352+
353+
// Specialization for types that appear to be copy constructible but also look like stl containers
354+
// (we specifically check for: has `value_type` and `reference` with `reference = value_type&`): if
355+
// so, copy constructability depends on whether the value_type is copy constructible.
356+
template <typename Container> struct is_copy_constructible<Container, enable_if_t<
357+
std::is_copy_constructible<Container>::value &&
358+
std::is_same<typename Container::value_type &, typename Container::reference>::value
359+
>> : std::is_copy_constructible<typename Container::value_type> {};
360+
349361
/// Generic type caster for objects stored on the heap
350362
template <typename type> class type_caster_base : public type_caster_generic {
351363
using itype = intrinsic_t<type>;
@@ -383,20 +395,21 @@ template <typename type> class type_caster_base : public type_caster_generic {
383395
#if !defined(_MSC_VER)
384396
/* Only enabled when the types are {copy,move}-constructible *and* when the type
385397
does not have a private operator new implementaton. */
386-
template <typename T = type> static auto make_copy_constructor(const T *value) -> decltype(new T(*value), Constructor(nullptr)) {
398+
template <typename T = type, typename = enable_if_t<is_copy_constructible<T>::value>> static auto make_copy_constructor(const T *value) -> decltype(new T(*value), Constructor(nullptr)) {
387399
return [](const void *arg) -> void * { return new T(*((const T *) arg)); }; }
388400
template <typename T = type> static auto make_move_constructor(const T *value) -> decltype(new T(std::move(*((T *) value))), Constructor(nullptr)) {
389401
return [](const void *arg) -> void * { return (void *) new T(std::move(*((T *) arg))); }; }
390402
#else
391403
/* Visual Studio 2015's SFINAE implementation doesn't yet handle the above robustly in all situations.
392404
Use a workaround that only tests for constructibility for now. */
393-
template <typename T = type, typename = enable_if_t<std::is_copy_constructible<T>::value>>
405+
template <typename T = type, typename = enable_if_t<is_copy_constructible<T>::value>>
394406
static Constructor make_copy_constructor(const T *value) {
395407
return [](const void *arg) -> void * { return new T(*((const T *)arg)); }; }
396408
template <typename T = type, typename = enable_if_t<std::is_move_constructible<T>::value>>
397409
static Constructor make_move_constructor(const T *value) {
398410
return [](const void *arg) -> void * { return (void *) new T(std::move(*((T *)arg))); }; }
399411
#endif
412+
400413
static Constructor make_copy_constructor(...) { return nullptr; }
401414
static Constructor make_move_constructor(...) { return nullptr; }
402415
};

0 commit comments

Comments
 (0)