Skip to content
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

Merged
merged 15 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 71 additions & 18 deletions include/pybind11/stl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -352,36 +355,64 @@ 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{});
// 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());

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)

Copy link
Contributor Author

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:

This PR preserves the existing array_caster behavior as much as possible. The only difference is that it also works for value types that are not default-constructible. — Note that without this PR, the new test results in a compiler error.

I also added a source code comment here:

        // Using `resize` to preserve the behavior exactly as it was before google/pywrapcc#30034

size_t ctr = 0;
for (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;
// 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;
}

Expand Down Expand Up @@ -420,13 +451,35 @@ 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.
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`.

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

@rwgk rwgk Jan 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added a source code comment here:

// Intentionally preserving the behavior exactly as it was before google/pywrapcc#30034

Do you know typically from whom is src allocated?

No! :-)

I think this is one of the dark corners of pybind11, specifically the PYBIND11_TYPE_CASTER macro, which is used a lot. But the ecosystem has grown around it ... better don't touch.

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>
Expand Down
17 changes: 17 additions & 0 deletions tests/test_stl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,23 @@ TEST_SUBMODULE(stl, m) {
m.def("cast_array", []() { return std::array<int, 2>{{1, 2}}; });
m.def("load_array", [](const std::array<int, 2> &a) { return a[0] == 1 && a[1] == 2; });

struct NoDefaultCtor {
explicit constexpr NoDefaultCtor(int val) : val{val} {}
int val;
};

struct NoDefaultCtorArray {
explicit constexpr NoDefaultCtorArray(int i)
: arr{{NoDefaultCtor(10 + i), NoDefaultCtor(20 + i)}} {}
std::array<NoDefaultCtor, 2> arr;
};

// test_array_no_default_ctor
py::class_<NoDefaultCtor>(m, "NoDefaultCtor").def_readonly("val", &NoDefaultCtor::val);
py::class_<NoDefaultCtorArray>(m, "NoDefaultCtorArray")
.def(py::init<int>())
.def_readwrite("arr", &NoDefaultCtorArray::arr);

// test_valarray
m.def("cast_valarray", []() { return std::valarray<int>{1, 4, 9}; });
m.def("load_valarray", [](const std::valarray<int> &v) {
Expand Down
7 changes: 7 additions & 0 deletions tests/test_stl.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array_caster copies (or moves) elements to/from a native Python list. The code below demonstrates the behavior. I verified that it works, then undid the change again, because that aspect of the array_caster behavior is tangential to adding the default ctor support.

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()
Expand Down
Loading