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).diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h
index adf7ea9..9c4b968 100644
--- a/include/pybind11/cast.h
+++ b/include/pybind11/cast.h
@@ -346,6 +346,18 @@
typename std::add_pointer<intrinsic_t<T>>::type,
typename std::add_lvalue_reference<intrinsic_t<T>>::type>::type;
+// std::is_copy_constructible isn't quite enough: it lets std::vector<T> (and similar) through when
+// T is non-copyable, but code containing such a copy constructor fails to actually compile.
+template <typename T, typename SFINAE = void> struct is_copy_constructible : std::is_copy_constructible<T> {};
+
+// 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 <typename Container> struct is_copy_constructible<Container, enable_if_t<
+ std::is_copy_constructible<Container>::value &&
+ std::is_same<typename Container::value_type &, typename Container::reference>::value
+ >> : std::is_copy_constructible<typename Container::value_type> {};
+
/// Generic type caster for objects stored on the heap
template <typename type> class type_caster_base : public type_caster_generic {
using itype = intrinsic_t<type>;
@@ -383,20 +395,21 @@
#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 <typename T = type> static auto make_copy_constructor(const T *value) -> decltype(new T(*value), Constructor(nullptr)) {
+ 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)) {
return [](const void *arg) -> void * { return new T(*((const T *) arg)); }; }
template <typename T = type> 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 <typename T = type, typename = enable_if_t<std::is_copy_constructible<T>::value>>
+ template <typename T = type, typename = enable_if_t<is_copy_constructible<T>::value>>
static Constructor make_copy_constructor(const T *value) {
return [](const void *arg) -> void * { return new T(*((const T *)arg)); }; }
template <typename T = type, typename = enable_if_t<std::is_move_constructible<T>::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 24963aa..ef9950e 100644
--- a/include/pybind11/stl_bind.h
+++ b/include/pybind11/stl_bind.h
@@ -60,18 +60,21 @@
};
/* Fallback functions */
-template <typename, typename, typename... Args> void vector_if_copy_constructible(const Args&...) { }
-template <typename, typename, typename... Args> void vector_if_equal_operator(const Args&...) { }
-template <typename, typename, typename... Args> void vector_if_insertion_operator(const Args&...) { }
+template <typename, typename, typename... Args> void vector_if_copy_constructible(const Args &...) { }
+template <typename, typename, typename... Args> void vector_if_equal_operator(const Args &...) { }
+template <typename, typename, typename... Args> void vector_if_insertion_operator(const Args &...) { }
+template <typename, typename, typename... Args> void vector_modifiers(const Args &...) { }
-template<typename Vector, typename Class_, enable_if_t<std::is_copy_constructible<typename Vector::value_type>::value, int> = 0>
-void vector_if_copy_constructible(Class_ &cl) {
- cl.def(pybind11::init<const Vector &>(),
- "Copy constructor");
+template<typename Vector, typename Class_>
+void vector_if_copy_constructible(enable_if_t<
+ std::is_copy_constructible<Vector>::value &&
+ std::is_copy_constructible<typename Vector::value_type>::value, Class_> &cl) {
+
+ cl.def(pybind11::init<const Vector &>(), "Copy constructor");
}
-template<typename Vector, typename Class_, enable_if_t<is_comparable<Vector>::value, int> = 0>
-void vector_if_equal_operator(Class_ &cl) {
+template<typename Vector, typename Class_>
+void vector_if_equal_operator(enable_if_t<is_comparable<Vector>::value, Class_> &cl) {
using T = typename Vector::value_type;
cl.def(self == self);
@@ -106,71 +109,34 @@
);
}
-template <typename Vector, typename Class_> auto vector_if_insertion_operator(Class_ &cl, std::string const &name)
- -> decltype(std::declval<std::ostream&>() << std::declval<typename Vector::value_type>(), 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 Vector, typename holder_type = std::unique_ptr<Vector>, typename... Args>
-pybind11::class_<Vector, holder_type> 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 <typename Vector, typename Class_>
+void vector_modifiers(enable_if_t<std::is_copy_constructible<typename Vector::value_type>::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_<Vector, holder_type>;
-
- Class_ cl(m, name.c_str(), std::forward<Args>(args)...);
-
- cl.def(pybind11::init<>());
-
- // Register copy constructor (if possible)
- detail::vector_if_copy_constructible<Vector, Class_>(cl);
-
- // Register comparison-related operators and functions (if possible)
- detail::vector_if_equal_operator<Vector, Class_>(cl);
-
- // Register stream insertion operator (if possible)
- detail::vector_if_insertion_operator<Vector, Class_>(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<typename Vector::value_type>());
- } 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<T>());
+ } 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 @@
"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 @@
}
);
- 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 * {
@@ -291,6 +222,15 @@
);
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 @@
"Delete list elements using a slice object"
);
+}
+
+// If the type has an operator[] that doesn't return a reference (most notably std::vector<bool>),
+// we have to access by copying; otherwise we return by reference.
+template <typename Vector> using vector_needs_copy = bool_constant<
+ !std::is_same<decltype(std::declval<Vector>()[typename Vector::size_type()]), typename Vector::value_type &>::value>;
+
+// The usual case: access and iterate by reference
+template <typename Vector, typename Class_>
+void vector_accessor(enable_if_t<!vector_needs_copy<Vector>::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<bool>, that have to be returned-by-copy:
+template <typename Vector, typename Class_>
+void vector_accessor(enable_if_t<vector_needs_copy<Vector>::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 <typename Vector, typename Class_> auto vector_if_insertion_operator(Class_ &cl, std::string const &name)
+ -> decltype(std::declval<std::ostream&>() << std::declval<typename Vector::value_type>(), 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 Vector, typename holder_type = std::unique_ptr<Vector>, typename... Args>
+pybind11::class_<Vector, holder_type> bind_vector(pybind11::module &m, std::string const &name, Args&&... args) {
+ using Class_ = pybind11::class_<Vector, holder_type>;
+
+ Class_ cl(m, name.c_str(), std::forward<Args>(args)...);
+
+ cl.def(pybind11::init<>());
+
+ // Register copy constructor (if possible)
+ detail::vector_if_copy_constructible<Vector, Class_>(cl);
+
+ // Register comparison-related operators and functions (if possible)
+ detail::vector_if_equal_operator<Vector, Class_>(cl);
+
+ // Register stream insertion operator (if possible)
+ detail::vector_if_insertion_operator<Vector, Class_>(cl, name);
+
+ // Modifiers require copyable vector value type
+ detail::vector_modifiers<Vector, Class_>(cl);
+
+ // Accessor and iterator; return by value if copyable, otherwise we return by ref + keep-alive
+ detail::vector_accessor<Vector, Class_>(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<size_type>());
@@ -361,9 +418,12 @@
NAMESPACE_BEGIN(detail)
/* Fallback functions */
-template <typename, typename, typename... Args> void map_if_insertion_operator(const Args&...) { }
+template <typename, typename, typename... Args> void map_if_insertion_operator(const Args &...) { }
+template <typename, typename, typename... Args> void map_assignment(const Args &...) { }
-template <typename Map, typename Class_, typename... Args> void map_if_copy_assignable(Class_ &cl, const Args&...) {
+// Map assignment when copy-assignable: just copy the value
+template <typename Map, typename Class_>
+void map_assignment(enable_if_t<std::is_copy_assignable<typename Map::mapped_type>::value, Class_> &cl) {
using KeyType = typename Map::key_type;
using MappedType = typename Map::mapped_type;
@@ -376,19 +436,23 @@
);
}
-template<typename Map, typename Class_, enable_if_t<!std::is_copy_assignable<typename Map::mapped_type>::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<typename Map, typename Class_>
+void map_assignment(enable_if_t<
+ !std::is_copy_assignable<typename Map::mapped_type>::value &&
+ std::is_copy_constructible<typename Map::mapped_type>::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 @@
"Return the canonical string representation of this map."
);
}
+
+
NAMESPACE_END(detail)
template <typename Map, typename holder_type = std::unique_ptr<Map>, typename... Args>
@@ -446,15 +512,17 @@
);
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<Map, Class_>(cl);
+ // Assignment provided only if the type is copyable
+ detail::map_assignment<Map, Class_>(cl);
cl.def("__delitem__",
[](Map &m, const KeyType &k) {
diff --git a/tests/test_stl_binders.cpp b/tests/test_stl_binders.cpp
index e390376..b9b56c1 100644
--- a/tests/test_stl_binders.cpp
+++ b/tests/test_stl_binders.cpp
@@ -11,6 +11,7 @@
#include <pybind11/stl_bind.h>
#include <map>
+#include <deque>
#include <unordered_map>
class El {
@@ -26,6 +27,32 @@
return s;
}
+/// Issue #487: binding std::vector<E> 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 <class Container> Container *one_to_n(int n) {
+ auto v = new Container();
+ for (int i = 1; i <= n; i++)
+ v->emplace_back(i);
+ return v;
+}
+
+template <class Map> 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_<El>(m, "El")
.def(py::init<int>());
@@ -36,6 +63,7 @@
py::bind_vector<std::vector<El>>(m, "VectorEl");
py::bind_vector<std::vector<std::vector<El>>>(m, "VectorVectorEl");
+
});
test_initializer stl_binder_map([](py::module &m) {
@@ -44,4 +72,24 @@
py::bind_map<std::map<std::string, double const>>(m, "MapStringDoubleConst");
py::bind_map<std::unordered_map<std::string, double const>>(m, "UnorderedMapStringDoubleConst");
+
});
+
+test_initializer stl_binder_noncopyable([](py::module &m) {
+ py::class_<E_nc>(m, "ENC")
+ .def(py::init<int>())
+ .def_readwrite("value", &E_nc::value);
+
+ py::bind_vector<std::vector<E_nc>>(m, "VectorENC");
+ m.def("get_vnc", &one_to_n<std::vector<E_nc>>, py::return_value_policy::reference);
+
+ py::bind_vector<std::deque<E_nc>>(m, "DequeENC");
+ m.def("get_dnc", &one_to_n<std::deque<E_nc>>, py::return_value_policy::reference);
+
+ py::bind_map<std::map<int, E_nc>>(m, "MapENC");
+ m.def("get_mnc", ×_ten<std::map<int, E_nc>>, py::return_value_policy::reference);
+
+ py::bind_map<std::unordered_map<int, E_nc>>(m, "UmapENC");
+ m.def("get_umnc", ×_ten<std::unordered_map<int, E_nc>>, py::return_value_policy::reference);
+});
+
diff --git a/tests/test_stl_binders.py b/tests/test_stl_binders.py
index 3026357..ba62e51 100644
--- a/tests/test_stl_binders.py
+++ b/tests/test_stl_binders.py
@@ -50,7 +50,6 @@
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 @@
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)
+