From 617fbcfc1e90b8cf9b98c6a25ba112b84ab16b9e Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Tue, 15 Nov 2016 06:30:38 -0500 Subject: [PATCH] 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 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--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). --- include/pybind11/cast.h | 17 ++- include/pybind11/stl_bind.h | 294 ++++++++++++++++++++++-------------- tests/test_stl_binders.cpp | 48 ++++++ tests/test_stl_binders.py | 56 ++++++- 4 files changed, 299 insertions(+), 116 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index adf7ea99f..9c4b9680b 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -346,6 +346,18 @@ using cast_op_type = typename std::conditional>::type, typename std::add_lvalue_reference>::type>::type; +// std::is_copy_constructible isn't quite enough: it lets std::vector (and similar) through when +// T is non-copyable, but code containing such a copy constructor fails to actually compile. +template struct is_copy_constructible : std::is_copy_constructible {}; + +// Specialization for types that appear to be copy constructible but also look like stl containers +// (we specifically check for: has `value_type` and `reference` with `reference = value_type&`): if +// so, copy constructability depends on whether the value_type is copy constructible. +template struct is_copy_constructible::value && + std::is_same::value + >> : std::is_copy_constructible {}; + /// Generic type caster for objects stored on the heap template class type_caster_base : public type_caster_generic { using itype = intrinsic_t; @@ -383,20 +395,21 @@ protected: #if !defined(_MSC_VER) /* Only enabled when the types are {copy,move}-constructible *and* when the type does not have a private operator new implementaton. */ - template static auto make_copy_constructor(const T *value) -> decltype(new T(*value), Constructor(nullptr)) { + template ::value>> static auto make_copy_constructor(const T *value) -> decltype(new T(*value), Constructor(nullptr)) { return [](const void *arg) -> void * { return new T(*((const T *) arg)); }; } template static auto make_move_constructor(const T *value) -> decltype(new T(std::move(*((T *) value))), Constructor(nullptr)) { return [](const void *arg) -> void * { return (void *) new T(std::move(*((T *) arg))); }; } #else /* Visual Studio 2015's SFINAE implementation doesn't yet handle the above robustly in all situations. Use a workaround that only tests for constructibility for now. */ - template ::value>> + template ::value>> static Constructor make_copy_constructor(const T *value) { return [](const void *arg) -> void * { return new T(*((const T *)arg)); }; } template ::value>> static Constructor make_move_constructor(const T *value) { return [](const void *arg) -> void * { return (void *) new T(std::move(*((T *)arg))); }; } #endif + static Constructor make_copy_constructor(...) { return nullptr; } static Constructor make_move_constructor(...) { return nullptr; } }; diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 24963aaa0..ef9950ebb 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -60,18 +60,21 @@ struct is_comparable::is_pair>> { }; /* Fallback functions */ -template void vector_if_copy_constructible(const Args&...) { } -template void vector_if_equal_operator(const Args&...) { } -template void vector_if_insertion_operator(const Args&...) { } +template void vector_if_copy_constructible(const Args &...) { } +template void vector_if_equal_operator(const Args &...) { } +template void vector_if_insertion_operator(const Args &...) { } +template void vector_modifiers(const Args &...) { } -template::value, int> = 0> -void vector_if_copy_constructible(Class_ &cl) { - cl.def(pybind11::init(), - "Copy constructor"); +template +void vector_if_copy_constructible(enable_if_t< + std::is_copy_constructible::value && + std::is_copy_constructible::value, Class_> &cl) { + + cl.def(pybind11::init(), "Copy constructor"); } -template::value, int> = 0> -void vector_if_equal_operator(Class_ &cl) { +template +void vector_if_equal_operator(enable_if_t::value, Class_> &cl) { using T = typename Vector::value_type; cl.def(self == self); @@ -106,71 +109,34 @@ void vector_if_equal_operator(Class_ &cl) { ); } -template auto vector_if_insertion_operator(Class_ &cl, std::string const &name) - -> decltype(std::declval() << std::declval(), void()) { - using size_type = typename Vector::size_type; - - cl.def("__repr__", - [name](Vector &v) { - std::ostringstream s; - s << name << '['; - for (size_type i=0; i < v.size(); ++i) { - s << v[i]; - if (i != v.size() - 1) - s << ", "; - } - s << ']'; - return s.str(); - }, - "Return the canonical string representation of this list." - ); -} - -NAMESPACE_END(detail) - -// -// std::vector -// -template , typename... Args> -pybind11::class_ bind_vector(pybind11::module &m, std::string const &name, Args&&... args) { +// Vector modifiers -- requires a copyable vector_type: +// (Technically, some of these (pop and __delitem__) don't actually require copyability, but it seems +// silly to allow deletion but not insertion, so include them here too.) +template +void vector_modifiers(enable_if_t::value, Class_> &cl) { using T = typename Vector::value_type; using SizeType = typename Vector::size_type; using DiffType = typename Vector::difference_type; - using ItType = typename Vector::iterator; - using Class_ = pybind11::class_; - - Class_ cl(m, name.c_str(), std::forward(args)...); - - cl.def(pybind11::init<>()); - - // Register copy constructor (if possible) - detail::vector_if_copy_constructible(cl); - - // Register comparison-related operators and functions (if possible) - detail::vector_if_equal_operator(cl); - - // Register stream insertion operator (if possible) - detail::vector_if_insertion_operator(cl, name); - - cl.def("__init__", [](Vector &v, iterable it) { - new (&v) Vector(); - try { - v.reserve(len(it)); - for (handle h : it) - v.push_back(h.cast()); - } catch (...) { - v.~Vector(); - throw; - } - }); cl.def("append", [](Vector &v, const T &value) { v.push_back(value); }, arg("x"), "Add an item to the end of the list"); + cl.def("__init__", [](Vector &v, iterable it) { + new (&v) Vector(); + try { + v.reserve(len(it)); + for (handle h : it) + v.push_back(h.cast()); + } catch (...) { + v.~Vector(); + throw; + } + }); + cl.def("extend", - [](Vector &v, Vector &src) { + [](Vector &v, const Vector &src) { v.reserve(v.size() + src.size()); v.insert(v.end(), src.begin(), src.end()); }, @@ -209,21 +175,6 @@ pybind11::class_ bind_vector(pybind11::module &m, std::stri "Remove and return the item at index ``i``" ); - cl.def("__bool__", - [](const Vector &v) -> bool { - return !v.empty(); - }, - "Check whether the list is nonempty" - ); - - cl.def("__getitem__", - [](const Vector &v, SizeType i) -> T { - if (i >= v.size()) - throw pybind11::index_error(); - return v[i]; - } - ); - cl.def("__setitem__", [](Vector &v, SizeType i, const T &t) { if (i >= v.size()) @@ -232,26 +183,6 @@ pybind11::class_ bind_vector(pybind11::module &m, std::stri } ); - cl.def("__delitem__", - [](Vector &v, SizeType i) { - if (i >= v.size()) - throw pybind11::index_error(); - v.erase(v.begin() + typename Vector::difference_type(i)); - }, - "Delete list elements using a slice object" - ); - - cl.def("__len__", &Vector::size); - - cl.def("__iter__", - [](Vector &v) { - return pybind11::make_iterator< - return_value_policy::reference_internal, ItType, ItType, T>( - v.begin(), v.end()); - }, - pybind11::keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */ - ); - /// Slicing protocol cl.def("__getitem__", [](const Vector &v, slice slice) -> Vector * { @@ -290,6 +221,15 @@ pybind11::class_ bind_vector(pybind11::module &m, std::stri "Assign list elements using a slice object" ); + cl.def("__delitem__", + [](Vector &v, SizeType i) { + if (i >= v.size()) + throw pybind11::index_error(); + v.erase(v.begin() + DiffType(i)); + }, + "Delete the list elements at index ``i``" + ); + cl.def("__delitem__", [](Vector &v, slice slice) { size_t start, stop, step, slicelength; @@ -309,6 +249,123 @@ pybind11::class_ bind_vector(pybind11::module &m, std::stri "Delete list elements using a slice object" ); +} + +// If the type has an operator[] that doesn't return a reference (most notably std::vector), +// we have to access by copying; otherwise we return by reference. +template using vector_needs_copy = bool_constant< + !std::is_same()[typename Vector::size_type()]), typename Vector::value_type &>::value>; + +// The usual case: access and iterate by reference +template +void vector_accessor(enable_if_t::value, Class_> &cl) { + using T = typename Vector::value_type; + using SizeType = typename Vector::size_type; + using ItType = typename Vector::iterator; + + cl.def("__getitem__", + [](Vector &v, SizeType i) -> T & { + if (i >= v.size()) + throw pybind11::index_error(); + return v[i]; + }, + return_value_policy::reference_internal // ref + keepalive + ); + + cl.def("__iter__", + [](Vector &v) { + return pybind11::make_iterator< + return_value_policy::reference_internal, ItType, ItType, T&>( + v.begin(), v.end()); + }, + keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */ + ); +} + +// The case for special objects, like std::vector, that have to be returned-by-copy: +template +void vector_accessor(enable_if_t::value, Class_> &cl) { + using T = typename Vector::value_type; + using SizeType = typename Vector::size_type; + using ItType = typename Vector::iterator; + cl.def("__getitem__", + [](const Vector &v, SizeType i) -> T { + if (i >= v.size()) + throw pybind11::index_error(); + return v[i]; + } + ); + + cl.def("__iter__", + [](Vector &v) { + return pybind11::make_iterator< + return_value_policy::copy, ItType, ItType, T>( + v.begin(), v.end()); + }, + keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */ + ); +} + +template auto vector_if_insertion_operator(Class_ &cl, std::string const &name) + -> decltype(std::declval() << std::declval(), void()) { + using size_type = typename Vector::size_type; + + cl.def("__repr__", + [name](Vector &v) { + std::ostringstream s; + s << name << '['; + for (size_type i=0; i < v.size(); ++i) { + s << v[i]; + if (i != v.size() - 1) + s << ", "; + } + s << ']'; + return s.str(); + }, + "Return the canonical string representation of this list." + ); +} + +NAMESPACE_END(detail) + +// +// std::vector +// +template , typename... Args> +pybind11::class_ bind_vector(pybind11::module &m, std::string const &name, Args&&... args) { + using Class_ = pybind11::class_; + + Class_ cl(m, name.c_str(), std::forward(args)...); + + cl.def(pybind11::init<>()); + + // Register copy constructor (if possible) + detail::vector_if_copy_constructible(cl); + + // Register comparison-related operators and functions (if possible) + detail::vector_if_equal_operator(cl); + + // Register stream insertion operator (if possible) + detail::vector_if_insertion_operator(cl, name); + + // Modifiers require copyable vector value type + detail::vector_modifiers(cl); + + // Accessor and iterator; return by value if copyable, otherwise we return by ref + keep-alive + detail::vector_accessor(cl); + + cl.def("__bool__", + [](const Vector &v) -> bool { + return !v.empty(); + }, + "Check whether the list is nonempty" + ); + + cl.def("__len__", &Vector::size); + + + + #if 0 // C++ style functions deprecated, leaving it here as an example cl.def(pybind11::init()); @@ -361,9 +418,12 @@ pybind11::class_ bind_vector(pybind11::module &m, std::stri NAMESPACE_BEGIN(detail) /* Fallback functions */ -template void map_if_insertion_operator(const Args&...) { } +template void map_if_insertion_operator(const Args &...) { } +template void map_assignment(const Args &...) { } -template void map_if_copy_assignable(Class_ &cl, const Args&...) { +// Map assignment when copy-assignable: just copy the value +template +void map_assignment(enable_if_t::value, Class_> &cl) { using KeyType = typename Map::key_type; using MappedType = typename Map::mapped_type; @@ -376,19 +436,23 @@ template void map_if_copy_assi ); } -template::value, int> = 0> -void map_if_copy_assignable(Class_ &cl) { +// Not copy-assignable, but still copy-constructible: we can update the value by erasing and reinserting +template +void map_assignment(enable_if_t< + !std::is_copy_assignable::value && + std::is_copy_constructible::value, + Class_> &cl) { using KeyType = typename Map::key_type; using MappedType = typename Map::mapped_type; cl.def("__setitem__", [](Map &m, const KeyType &k, const MappedType &v) { // We can't use m[k] = v; because value type might not be default constructable - auto r = m.insert(std::make_pair(k, v)); + auto r = m.emplace(k, v); if (!r.second) { - // value type might be const so the only way to insert it is to erase it first... + // value type is not copy assignable so the only way to insert it is to erase it first... m.erase(r.first); - m.insert(std::make_pair(k, v)); + m.emplace(k, v); } } ); @@ -415,6 +479,8 @@ template auto map_if_insertion_operator(Class_ & "Return the canonical string representation of this map." ); } + + NAMESPACE_END(detail) template , typename... Args> @@ -446,15 +512,17 @@ pybind11::class_ bind_map(module &m, const std::string &name, ); cl.def("__getitem__", - [](Map &m, const KeyType &k) -> MappedType { - auto it = m.find(k); - if (it == m.end()) - throw pybind11::key_error(); - return it->second; - } + [](Map &m, const KeyType &k) -> MappedType & { + auto it = m.find(k); + if (it == m.end()) + throw pybind11::key_error(); + return it->second; + }, + return_value_policy::reference_internal // ref + keepalive ); - detail::map_if_copy_assignable(cl); + // Assignment provided only if the type is copyable + detail::map_assignment(cl); cl.def("__delitem__", [](Map &m, const KeyType &k) { diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp index e390376dc..b9b56c15d 100644 --- a/tests/test_stl_binders.cpp +++ b/tests/test_stl_binders.cpp @@ -11,6 +11,7 @@ #include #include +#include #include class El { @@ -26,6 +27,32 @@ std::ostream & operator<<(std::ostream &s, El const&v) { return s; } +/// Issue #487: binding std::vector with E non-copyable +class E_nc { +public: + explicit E_nc(int i) : value{i} {} + E_nc(const E_nc &) = delete; + E_nc &operator=(const E_nc &) = delete; + E_nc(E_nc &&) = default; + E_nc &operator=(E_nc &&) = default; + + int value; +}; + +template Container *one_to_n(int n) { + auto v = new Container(); + for (int i = 1; i <= n; i++) + v->emplace_back(i); + return v; +} + +template Map *times_ten(int n) { + auto m = new Map(); + for (int i = 1; i <= n; i++) + m->emplace(int(i), E_nc(10*i)); + return m; +} + test_initializer stl_binder_vector([](py::module &m) { py::class_(m, "El") .def(py::init()); @@ -36,6 +63,7 @@ test_initializer stl_binder_vector([](py::module &m) { py::bind_vector>(m, "VectorEl"); py::bind_vector>>(m, "VectorVectorEl"); + }); test_initializer stl_binder_map([](py::module &m) { @@ -44,4 +72,24 @@ test_initializer stl_binder_map([](py::module &m) { py::bind_map>(m, "MapStringDoubleConst"); py::bind_map>(m, "UnorderedMapStringDoubleConst"); + }); + +test_initializer stl_binder_noncopyable([](py::module &m) { + py::class_(m, "ENC") + .def(py::init()) + .def_readwrite("value", &E_nc::value); + + py::bind_vector>(m, "VectorENC"); + m.def("get_vnc", &one_to_n>, py::return_value_policy::reference); + + py::bind_vector>(m, "DequeENC"); + m.def("get_dnc", &one_to_n>, py::return_value_policy::reference); + + py::bind_map>(m, "MapENC"); + m.def("get_mnc", ×_ten>, py::return_value_policy::reference); + + py::bind_map>(m, "UmapENC"); + m.def("get_umnc", ×_ten>, py::return_value_policy::reference); +}); + diff --git a/tests/test_stl_binders.py b/tests/test_stl_binders.py index 302635715..ba62e515d 100644 --- a/tests/test_stl_binders.py +++ b/tests/test_stl_binders.py @@ -50,7 +50,6 @@ def test_vector_bool(): assert vv_c[i] == (i % 2 == 0) assert str(vv_c) == "VectorBool[1, 0, 1, 0, 1, 0, 1, 0, 1, 0]" - def test_map_string_double(): from pybind11_tests import MapStringDouble, UnorderedMapStringDouble @@ -97,3 +96,58 @@ def test_map_string_double_const(): umc['b'] = 21.5 str(umc) + +def test_noncopyable_vector(): + from pybind11_tests import ENC, get_vnc + + vnc = get_vnc(5) + for i in range(0, 5): + assert(vnc[i].value == i+1) + + i = 1 + for j in vnc: + assert(j.value == i) + i += 1 + +def test_noncopyable_deque(): + from pybind11_tests import ENC, get_dnc + + dnc = get_dnc(5) + for i in range(0, 5): + assert(dnc[i].value == i+1) + + i = 1 + for j in dnc: + assert(j.value == i) + i += 1 + +def test_noncopyable_map(): + from pybind11_tests import ENC, get_mnc + + mnc = get_mnc(5) + for i in range(1, 6): + assert(mnc[i].value == 10*i) + + i = 1 + vsum = 0 + for k, v in mnc.items(): + assert(v.value == 10*k) + vsum += v.value + + assert(vsum == 150) + +def test_noncopyable_unordered_map(): + from pybind11_tests import ENC, get_umnc + + mnc = get_umnc(5) + for i in range(1, 6): + assert(mnc[i].value == 10*i) + + i = 1 + vsum = 0 + for k, v in mnc.items(): + assert(v.value == 10*k) + vsum += v.value + + assert(vsum == 150) +