From cba08f1282da0927c43614ad5b3e89749c13525f Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Fri, 11 Nov 2016 21:44:00 -0500 Subject: [PATCH 1/6] Simplified stl_bind.h enable_if_t templates This removes some unncessary extra template parameters and parameter packs, making the logic a bit simpler. --- include/pybind11/stl_bind.h | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 24963aaa02..c8f41426d0 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -60,18 +60,19 @@ 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::value, int> = 0> -void vector_if_copy_constructible(Class_ &cl) { - cl.def(pybind11::init(), - "Copy constructor"); +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(enable_if_t< + 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); @@ -361,9 +362,10 @@ 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_if_copy_assignable(Class_ &cl, const Args&...) { +template +void map_assignment(enable_if_t::value, Class_> &cl) { using KeyType = typename Map::key_type; using MappedType = typename Map::mapped_type; @@ -376,8 +378,10 @@ template void map_if_copy_assi ); } -template::value, int> = 0> -void map_if_copy_assignable(Class_ &cl) { +template +void map_if_copy_assignable(enable_if_t< + !std::is_copy_assignable::value, + Class_> &cl) { using KeyType = typename Map::key_type; using MappedType = typename Map::mapped_type; From 43755c3ab9f840ac96532cedb69e8c210e0abf39 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Fri, 11 Nov 2016 21:45:27 -0500 Subject: [PATCH 2/6] 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. --- include/pybind11/cast.h | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index ddc1890c1a..d7092ab4f8 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -329,6 +329,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; @@ -366,20 +378,21 @@ template class type_caster_base : public type_caster_generic { #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; } }; From 378debbc02e070f98af2b9dbd7297fa403845350 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Fri, 11 Nov 2016 21:50:13 -0500 Subject: [PATCH 3/6] 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. --- include/pybind11/stl_bind.h | 282 +++++++++++++++++++++++------------- tests/test_stl_binders.cpp | 48 ++++++ tests/test_stl_binders.py | 56 ++++++- 3 files changed, 287 insertions(+), 99 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index c8f41426d0..28059deae9 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -63,9 +63,11 @@ struct is_comparable::is_pair>> { 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 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"); @@ -107,71 +109,34 @@ void vector_if_equal_operator(enable_if_t::value, Class_> ); } -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("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()); + 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("extend", - [](Vector &v, Vector &src) { + [](Vector &v, const Vector &src) { v.reserve(v.size() + src.size()); v.insert(v.end(), src.begin(), src.end()); }, @@ -210,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()) @@ -233,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 * { @@ -291,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; @@ -310,6 +249,118 @@ pybind11::class_ bind_vector(pybind11::module &m, std::stri "Delete list elements using a slice object" ); +} + +// Default __getitem__, when we can copy the value: +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::reference_internal, ItType, ItType, T>( + v.begin(), v.end()); + }, + keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */ + ); +} + +// When we can't copy, we have to return a reference and use a keepalive: +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 */ + ); +} + +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()); @@ -363,7 +414,9 @@ NAMESPACE_BEGIN(detail) /* Fallback functions */ template void map_if_insertion_operator(const Args &...) { } +template void map_assignment(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; @@ -378,9 +431,11 @@ void map_assignment(enable_if_t -void map_if_copy_assignable(enable_if_t< - !std::is_copy_assignable::value, +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; @@ -388,11 +443,11 @@ void map_if_copy_assignable(enable_if_t< 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); } } ); @@ -419,12 +474,48 @@ template auto map_if_insertion_operator(Class_ & "Return the canonical string representation of this map." ); } + +// Default __getitem__, when we can copy the value: +template +void map_accessor(enable_if_t::value, Class_> &cl) { + using KeyType = typename Map::key_type; + using MappedType = typename Map::mapped_type; + + 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; + } + ); + +} + +// When we can't copy, we have to return a reference and use a keepalive: +template +void map_accessor(enable_if_t::value, Class_> &cl) { + using KeyType = typename Map::key_type; + using MappedType = typename Map::mapped_type; + + 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; + }, + return_value_policy::reference_internal // ref + keepalive + ); + +} + + NAMESPACE_END(detail) template , typename... Args> pybind11::class_ bind_map(module &m, const std::string &name, Args&&... args) { using KeyType = typename Map::key_type; - using MappedType = typename Map::mapped_type; using Class_ = pybind11::class_; Class_ cl(m, name.c_str(), std::forward(args)...); @@ -449,16 +540,11 @@ pybind11::class_ bind_map(module &m, const std::string &name, pybind11::keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */ ); - 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; - } - ); + // Accessor and iterator; return by value if copyable, otherwise we return by ref + keep-alive + detail::map_accessor(cl); - 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 e390376dcf..b9b56c15d8 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 3026357158..ba62e515d5 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) + From a900d205b662b83a8ed20636f1e98140f7b78b42 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Sat, 12 Nov 2016 21:30:13 -0500 Subject: [PATCH 4/6] 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. --- include/pybind11/stl_bind.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 28059deae9..e2c58a3c18 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -251,9 +251,10 @@ void vector_modifiers(enable_if_t has to be done this way--its operator[] doesn't return a `bool&`): template -void vector_accessor(enable_if_t::value, Class_> &cl) { +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; @@ -268,16 +269,16 @@ void vector_accessor(enable_if_t( + return_value_policy::copy, ItType, ItType, T>( v.begin(), v.end()); }, keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */ ); } -// When we can't copy, we have to return a reference and use a keepalive: +// For any non-primitive types, we return by reference with a keepalive: template -void vector_accessor(enable_if_t::value, Class_> &cl) { +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; @@ -475,9 +476,9 @@ template auto map_if_insertion_operator(Class_ & ); } -// Default __getitem__, when we can copy the value: +// If the mapped type is copyable and a basic arithmetic C++ type, we return by copying template -void map_accessor(enable_if_t::value, Class_> &cl) { +void map_accessor(enable_if_t::value, Class_> &cl) { using KeyType = typename Map::key_type; using MappedType = typename Map::mapped_type; @@ -492,9 +493,9 @@ void map_accessor(enable_if_t -void map_accessor(enable_if_t::value, Class_> &cl) { +void map_accessor(enable_if_t::value, Class_> &cl) { using KeyType = typename Map::key_type; using MappedType = typename Map::mapped_type; From 848616be220fe191a1319e34a6adb73ccff17a03 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Sun, 13 Nov 2016 18:06:09 -0500 Subject: [PATCH 5/6] Return by reference whenever possible Only if we definitely can't--i.e. std::vector--becasue 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/stl_bind.h | 61 ++++++++++++------------------------- 1 file changed, 19 insertions(+), 42 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index e2c58a3c18..54928b18b0 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -251,10 +251,14 @@ void vector_modifiers(enable_if_t has to be done this way--its operator[] doesn't return a `bool&`): +// 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 case for special objects, like std::vector, that have to be returned-by-copy: template -void vector_accessor(enable_if_t::value, Class_> &cl) { +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; @@ -276,9 +280,9 @@ void vector_accessor(enable_if_t ); } -// For any non-primitive types, we return by reference with a keepalive: +// The usual case: access and iterate by reference template -void vector_accessor(enable_if_t::value, Class_> &cl) { +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; @@ -476,47 +480,13 @@ template auto map_if_insertion_operator(Class_ & ); } -// If the mapped type is copyable and a basic arithmetic C++ type, we return by copying -template -void map_accessor(enable_if_t::value, Class_> &cl) { - using KeyType = typename Map::key_type; - using MappedType = typename Map::mapped_type; - - 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; - } - ); - -} - -// For complex types, and for non-copyable types, we return by reference with a keep-alive -template -void map_accessor(enable_if_t::value, Class_> &cl) { - using KeyType = typename Map::key_type; - using MappedType = typename Map::mapped_type; - - 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; - }, - return_value_policy::reference_internal // ref + keepalive - ); - -} - NAMESPACE_END(detail) template , typename... Args> pybind11::class_ bind_map(module &m, const std::string &name, Args&&... args) { using KeyType = typename Map::key_type; + using MappedType = typename Map::mapped_type; using Class_ = pybind11::class_; Class_ cl(m, name.c_str(), std::forward(args)...); @@ -541,8 +511,15 @@ pybind11::class_ bind_map(module &m, const std::string &name, pybind11::keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */ ); - // Accessor and iterator; return by value if copyable, otherwise we return by ref + keep-alive - detail::map_accessor(cl); + 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; + }, + return_value_policy::reference_internal // ref + keepalive + ); // Assignment provided only if the type is copyable detail::map_assignment(cl); From e40ff5b1bb537ad05443aaf927d5b77e061ef691 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Sun, 13 Nov 2016 18:10:28 -0500 Subject: [PATCH 6/6] Cosmetic change: put usual vector case before exceptional case --- include/pybind11/stl_bind.h | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 54928b18b0..ef9950ebb1 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -256,50 +256,50 @@ void vector_modifiers(enable_if_t using vector_needs_copy = bool_constant< !std::is_same()[typename Vector::size_type()]), typename Vector::value_type &>::value>; -// The case for special objects, like std::vector, that have to be returned-by-copy: +// The usual case: access and iterate by reference template -void vector_accessor(enable_if_t::value, Class_> &cl) { +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 { + [](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::copy, ItType, ItType, T>( + return_value_policy::reference_internal, ItType, ItType, T&>( v.begin(), v.end()); }, keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */ ); } -// The usual case: access and iterate by reference +// 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) { +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 & { + [](const 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&>( + return_value_policy::copy, ItType, ItType, T>( v.begin(), v.end()); }, keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */