Skip to content

Adjusting type_caster<std::reference_wrapper<T>> to support const/non-const propagation in cast_op. #2705

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

Merged
merged 31 commits into from
Dec 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
65abb62
Allow type_caster of std::reference_wrapper<T> to be the same as a na…
laramiel Nov 30, 2020
c603907
Add tests/examples for std::reference_wrapper<const T>
laramiel Nov 30, 2020
5cdc0ca
Add tests which use mutable/immutable variants
laramiel Dec 2, 2020
35e7fa5
Add/finish tests that distinguish const& from &
laramiel Dec 2, 2020
4cd06d3
Add passing a const to non-const method.
laramiel Dec 2, 2020
5abd823
Demonstrate non-const conversion of reference_wrapper in tests.
laramiel Dec 2, 2020
c5bd104
Fix build errors from presubmit checks.
laramiel Dec 2, 2020
f0e41f5
Try and fix a few more CI errors
laramiel Dec 2, 2020
b56a568
More CI fixes.
laramiel Dec 2, 2020
d71cd7a
More CI fixups.
laramiel Dec 2, 2020
3c69635
Try and get PyPy to work.
laramiel Dec 2, 2020
8f32ba3
Additional minor fixups. Getting close to CI green.
laramiel Dec 2, 2020
d144e59
More ci fixes?
laramiel Dec 2, 2020
a7ec72e
fix clang-tidy warnings from presubmit
laramiel Dec 2, 2020
e1aca4b
fix more clang-tidy warnings
laramiel Dec 2, 2020
005d3fe
minor comment and consistency cleanups
laramiel Dec 2, 2020
4ac23ca
PyDECREF -> Py_DECREF
laramiel Dec 2, 2020
a3d9b18
copy/move constructors
laramiel Dec 2, 2020
5b4fb98
Resolve codereview comments
laramiel Dec 3, 2020
8dcbc42
more review comment fixes
laramiel Dec 3, 2020
9f77862
review comments: remove spurious &
laramiel Dec 7, 2020
7672e92
Make the test fail even when the static_assert is commented out.
laramiel Dec 8, 2020
c747ae2
apply presubmit formatting
laramiel Dec 8, 2020
33f57cc
Revert inclusion of test_freezable_type_caster
laramiel Dec 12, 2020
d869976
Add a test that validates const references propagation.
laramiel Dec 14, 2020
f109edd
mend
laramiel Dec 14, 2020
775a7c9
Review comments based changes.
laramiel Dec 14, 2020
6f6dac7
formatted files again.
laramiel Dec 14, 2020
fd6dc5b
Move const_ref_caster test to builtin_casters
laramiel Dec 14, 2020
b3ecc53
Review comments: use cast_op and adjust some comments.
laramiel Dec 15, 2020
c22772b
Simplify ConstRefCasted test
laramiel Dec 15, 2020
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
13 changes: 9 additions & 4 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -960,9 +960,14 @@ template <typename type> class type_caster<std::reference_wrapper<type>> {
private:
using caster_t = make_caster<type>;
caster_t subcaster;
using subcaster_cast_op_type = typename caster_t::template cast_op_type<type>;
static_assert(std::is_same<typename std::remove_const<type>::type &, subcaster_cast_op_type>::value,
"std::reference_wrapper<T> caster requires T to have a caster with an `T &` operator");
using reference_t = type&;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could still be collapsed where it's used, to be shorter and more transparent (which was why I suggested type & instead of add_lvalue_reference<type>::type). But if that's the only thing left to change, I'm fine with this as well, and you don't need to add yet another commits, AFAIC.

using subcaster_cast_op_type =
typename caster_t::template cast_op_type<reference_t>;

static_assert(std::is_same<typename std::remove_const<type>::type &, subcaster_cast_op_type>::value ||
std::is_same<reference_t, subcaster_cast_op_type>::value,
"std::reference_wrapper<T> caster requires T to have a caster with an "
"`operator T &()` or `operator const T &()`");
public:
bool load(handle src, bool convert) { return subcaster.load(src, convert); }
static constexpr auto name = caster_t::name;
Expand All @@ -973,7 +978,7 @@ template <typename type> class type_caster<std::reference_wrapper<type>> {
return caster_t::cast(&src.get(), policy, parent);
}
template <typename T> using cast_op_type = std::reference_wrapper<type>;
operator std::reference_wrapper<type>() { return subcaster.operator subcaster_cast_op_type&(); }
operator std::reference_wrapper<type>() { return cast_op<type &>(subcaster); }
};

#define PYBIND11_TYPE_CASTER(type, py_name) \
Expand Down
64 changes: 64 additions & 0 deletions tests/test_builtin_casters.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,49 @@
# pragma warning(disable: 4127) // warning C4127: Conditional expression is constant
#endif

struct ConstRefCasted {
int tag;
};

PYBIND11_NAMESPACE_BEGIN(pybind11)
PYBIND11_NAMESPACE_BEGIN(detail)
template <>
class type_caster<ConstRefCasted> {
public:
static constexpr auto name = _<ConstRefCasted>();

// Input is unimportant, a new value will always be constructed based on the
// cast operator.
bool load(handle, bool) { return true; }

operator ConstRefCasted&&() { value = {1}; return std::move(value); }
operator ConstRefCasted&() { value = {2}; return value; }
operator ConstRefCasted*() { value = {3}; return &value; }

operator const ConstRefCasted&() { value = {4}; return value; }
operator const ConstRefCasted*() { value = {5}; return &value; }

// custom cast_op to explicitly propagate types to the conversion operators.
template <typename T_>
using cast_op_type =
/// const
conditional_t<
std::is_same<remove_reference_t<T_>, const ConstRefCasted*>::value, const ConstRefCasted*,
conditional_t<
std::is_same<T_, const ConstRefCasted&>::value, const ConstRefCasted&,
/// non-const
conditional_t<
std::is_same<remove_reference_t<T_>, ConstRefCasted*>::value, ConstRefCasted*,
conditional_t<
std::is_same<T_, ConstRefCasted&>::value, ConstRefCasted&,
/* else */ConstRefCasted&&>>>>;

private:
ConstRefCasted value = {0};
};
PYBIND11_NAMESPACE_END(detail)
PYBIND11_NAMESPACE_END(pybind11)

TEST_SUBMODULE(builtin_casters, m) {
// test_simple_string
m.def("string_roundtrip", [](const char *s) { return s; });
Expand Down Expand Up @@ -147,6 +190,17 @@ TEST_SUBMODULE(builtin_casters, m) {
// test_reference_wrapper
m.def("refwrap_builtin", [](std::reference_wrapper<int> p) { return 10 * p.get(); });
m.def("refwrap_usertype", [](std::reference_wrapper<UserType> p) { return p.get().value(); });
m.def("refwrap_usertype_const", [](std::reference_wrapper<const UserType> p) { return p.get().value(); });

m.def("refwrap_lvalue", []() -> std::reference_wrapper<UserType> {
static UserType x(1);
return std::ref(x);
});
m.def("refwrap_lvalue_const", []() -> std::reference_wrapper<const UserType> {
static UserType x(1);
return std::cref(x);
});

// Not currently supported (std::pair caster has return-by-value cast operator);
// triggers static_assert failure.
//m.def("refwrap_pair", [](std::reference_wrapper<std::pair<int, int>>) { });
Expand Down Expand Up @@ -189,4 +243,14 @@ TEST_SUBMODULE(builtin_casters, m) {
py::object o = py::cast(v);
return py::cast<void *>(o) == v;
});

// Tests const/non-const propagation in cast_op.
m.def("takes", [](ConstRefCasted x) { return x.tag; });
m.def("takes_move", [](ConstRefCasted&& x) { return x.tag; });
m.def("takes_ptr", [](ConstRefCasted* x) { return x->tag; });
m.def("takes_ref", [](ConstRefCasted& x) { return x.tag; });
m.def("takes_ref_wrap", [](std::reference_wrapper<ConstRefCasted> x) { return x.get().tag; });
m.def("takes_const_ptr", [](const ConstRefCasted* x) { return x->tag; });
m.def("takes_const_ref", [](const ConstRefCasted& x) { return x.tag; });
m.def("takes_const_ref_wrap", [](std::reference_wrapper<const ConstRefCasted> x) { return x.get().tag; });
}
22 changes: 22 additions & 0 deletions tests/test_builtin_casters.py
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ def test_reference_wrapper():
"""std::reference_wrapper for builtin and user types"""
assert m.refwrap_builtin(42) == 420
assert m.refwrap_usertype(UserType(42)) == 42
assert m.refwrap_usertype_const(UserType(42)) == 42

with pytest.raises(TypeError) as excinfo:
m.refwrap_builtin(None)
Expand All @@ -324,6 +325,9 @@ def test_reference_wrapper():
m.refwrap_usertype(None)
assert "incompatible function arguments" in str(excinfo.value)

assert m.refwrap_lvalue().value == 1
assert m.refwrap_lvalue_const().value == 1

a1 = m.refwrap_list(copy=True)
a2 = m.refwrap_list(copy=True)
assert [x.value for x in a1] == [2, 3]
Expand Down Expand Up @@ -421,3 +425,21 @@ def test_int_long():

def test_void_caster_2():
assert m.test_void_caster()


def test_const_ref_caster():
"""Verifies that const-ref is propagated through type_caster cast_op.
The returned ConstRefCasted type is a mimimal type that is constructed to
reference the casting mode used.
"""
x = False
assert m.takes(x) == 1
assert m.takes_move(x) == 1

assert m.takes_ptr(x) == 3
assert m.takes_ref(x) == 2
assert m.takes_ref_wrap(x) == 2

assert m.takes_const_ptr(x) == 5
assert m.takes_const_ref(x) == 4
assert m.takes_const_ref_wrap(x) == 4