-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change array_caster
in pybind11/stl.h to support value types that are not default-constructible.
#30034
Change array_caster
in pybind11/stl.h to support value types that are not default-constructible.
#30034
Changes from all commits
827e167
85a6077
cca21ee
3d61895
aa82e3a
e26769a
080fa2b
6dd0c70
cb0021c
d2e1222
3f14ae3
64dcab7
f90342a
8486c9e
7c45242
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,11 +11,14 @@ | |
|
||
#include "pybind11.h" | ||
#include "detail/common.h" | ||
#include "detail/descr.h" | ||
#include "detail/type_caster_base.h" | ||
|
||
#include <deque> | ||
#include <initializer_list> | ||
#include <list> | ||
#include <map> | ||
#include <memory> | ||
#include <ostream> | ||
#include <set> | ||
#include <unordered_map> | ||
|
@@ -352,36 +355,65 @@ struct type_caster<std::deque<Type, Alloc>> : list_caster<std::deque<Type, Alloc | |
template <typename Type, typename Alloc> | ||
struct type_caster<std::list<Type, Alloc>> : list_caster<std::list<Type, Alloc>, Type> {}; | ||
|
||
template <typename ArrayType, typename V, size_t... I> | ||
ArrayType vector_to_array_impl(V &&v, index_sequence<I...>) { | ||
return {{std::move(v[I])...}}; | ||
} | ||
|
||
// Based on https://en.cppreference.com/w/cpp/container/array/to_array | ||
template <typename ArrayType, size_t N, typename V> | ||
ArrayType vector_to_array(V &&v) { | ||
return vector_to_array_impl<ArrayType, V>(std::forward<V>(v), make_index_sequence<N>{}); | ||
} | ||
|
||
template <typename ArrayType, typename Value, bool Resizable, size_t Size = 0> | ||
struct array_caster { | ||
using value_conv = make_caster<Value>; | ||
|
||
private: | ||
template <bool R = Resizable> | ||
bool require_size(enable_if_t<R, size_t> size) { | ||
if (value.size() != size) { | ||
value.resize(size); | ||
std::unique_ptr<ArrayType> value; | ||
|
||
template <bool R = Resizable, enable_if_t<R, int> = 0> | ||
bool convert_elements(handle seq, bool convert) { | ||
auto l = reinterpret_borrow<sequence>(seq); | ||
value.reset(new ArrayType{}); | ||
// Using `resize` to preserve the behavior exactly as it was before google/pywrapcc#30034 | ||
// For the `resize` to work, `Value` must be default constructible. | ||
// For `std::valarray`, this is a requirement: | ||
// https://en.cppreference.com/w/cpp/named_req/NumericType | ||
value->resize(l.size()); | ||
size_t ctr = 0; | ||
for (const auto &it : l) { | ||
value_conv conv; | ||
if (!conv.load(it, convert)) { | ||
return false; | ||
} | ||
(*value)[ctr++] = cast_op<Value &&>(std::move(conv)); | ||
} | ||
return true; | ||
} | ||
template <bool R = Resizable> | ||
bool require_size(enable_if_t<!R, size_t> size) { | ||
return size == Size; | ||
} | ||
|
||
template <bool R = Resizable, enable_if_t<!R, int> = 0> | ||
bool convert_elements(handle seq, bool convert) { | ||
auto l = reinterpret_borrow<sequence>(seq); | ||
if (!require_size(l.size())) { | ||
if (l.size() != Size) { | ||
return false; | ||
} | ||
size_t ctr = 0; | ||
for (const auto &it : l) { | ||
// The `temp` storage is needed to support `Value` types that are not | ||
// default-constructible. | ||
// Deliberate choice: no template specializations, for simplicity, and | ||
// because the compile time overhead for the specializations is deemed | ||
// more significant than the runtime overhead for the `temp` storage. | ||
std::vector<Value> temp; | ||
temp.reserve(l.size()); | ||
for (auto it : l) { | ||
value_conv conv; | ||
if (!conv.load(it, convert)) { | ||
return false; | ||
} | ||
value[ctr++] = cast_op<Value &&>(std::move(conv)); | ||
temp.emplace_back(cast_op<Value &&>(std::move(conv))); | ||
} | ||
value.reset(new ArrayType(vector_to_array<ArrayType, Size>(std::move(temp)))); | ||
return true; | ||
} | ||
|
||
|
@@ -420,13 +452,36 @@ struct array_caster { | |
return l.release(); | ||
} | ||
|
||
PYBIND11_TYPE_CASTER_RVPP(ArrayType, | ||
const_name<Resizable>(const_name(""), const_name("Annotated[")) | ||
+ const_name("list[") + value_conv::name + const_name("]") | ||
+ const_name<Resizable>(const_name(""), | ||
const_name(", FixedSize(") | ||
+ const_name<Size>() | ||
+ const_name(")]"))); | ||
// Code copied from PYBIND11_TYPE_CASTER macro. | ||
// Intentionally preserving the behavior exactly as it was before google/pywrapcc#30034 | ||
template <typename T_, enable_if_t<std::is_same<ArrayType, remove_cv_t<T_>>::value, int> = 0> | ||
static handle cast(T_ *src, const return_value_policy_pack &policy, handle parent) { | ||
if (!src) { | ||
return none().release(); | ||
} | ||
if (policy == return_value_policy::take_ownership) { | ||
auto h = cast(std::move(*src), policy, parent); | ||
delete src; // WARNING: Assumes `src` was allocated with `new`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you know typically from whom is src allocated? If so, might be helpful to mention that here for more context. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or is this a typical requirement for any take_ownership policy on casters with pointer type src? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also added a source code comment here:
No! :-) I think this is one of the dark corners of pybind11, specifically the |
||
return h; | ||
} | ||
return cast(*src, policy, parent); | ||
} | ||
|
||
// NOLINTNEXTLINE(google-explicit-constructor) | ||
operator ArrayType *() { return &(*value); } | ||
// NOLINTNEXTLINE(google-explicit-constructor) | ||
operator ArrayType &() { return *value; } | ||
// NOLINTNEXTLINE(google-explicit-constructor) | ||
operator ArrayType &&() && { return std::move(*value); } | ||
|
||
template <typename T_> | ||
using cast_op_type = movable_cast_op_type<T_>; | ||
|
||
static constexpr auto name | ||
= const_name<Resizable>(const_name(""), const_name("Annotated[")) + const_name("list[") | ||
+ value_conv::name + const_name("]") | ||
+ const_name<Resizable>( | ||
const_name(""), const_name(", FixedSize(") + const_name<Size>() + const_name(")]")); | ||
}; | ||
|
||
template <typename Type, size_t Size> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,13 @@ def test_array(doc): | |
) | ||
|
||
|
||
def test_array_no_default_ctor(): | ||
lst = m.NoDefaultCtorArray(3) | ||
assert [e.val for e in lst.arr] == [13, 23] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you allow mutation of members of the array? if so might want to add a test about whether mutating lst.arr affects m.NoDefaultCtorArray(4).arr. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The def test_array_no_default_ctor():
lst = m.NoDefaultCtorArray(3)
assert [e.val for e in lst.arr] == [13, 23]
lst4 = m.NoDefaultCtorArray(4)
lst.arr = lst4.arr
assert [e.val for e in lst.arr] == [14, 24]
# Tangential to default ctor behavior:
lst.arr = m.NoDefaultCtorArray(5).arr
assert [e.val for e in lst.arr] == [15, 25]
assert [e.val for e in lst4.arr] == [14, 24] |
||
lst.arr = m.NoDefaultCtorArray(4).arr | ||
assert [e.val for e in lst.arr] == [14, 24] | ||
|
||
|
||
def test_valarray(doc): | ||
"""std::valarray <-> list""" | ||
lst = m.cast_valarray() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to use e.g. emplace to construct the elements in place? then you can likely avoid the default constructible requirement. (not sure if the comment above means the requirement is a limitation or always satisfied)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I added this to the PR description after seeing your questions:
I also added a source code comment here: