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 18 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
12 changes: 9 additions & 3 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -960,9 +960,15 @@ 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 = typename std::add_lvalue_reference<type>::type;
Copy link
Collaborator

@YannickJadoul YannickJadoul Dec 14, 2020

Choose a reason for hiding this comment

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

Why not just type &? Apparently the only difference is void, but I don't think std::reference_wrapper<void> is supported, or is it?

Copy link
Collaborator

@YannickJadoul YannickJadoul Dec 14, 2020

Choose a reason for hiding this comment

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

To be more specific, type_caster<std::reference_wrapper<void>> would already result in a compilation error a few lines further, to evaluate std::is_same<typename std::remove_const<type>::type &, subcaster_cast_op_type>::value: typename std::remove_const<void>::type is still void, and would also result in void &, and thus a compilation error.

According to cppreference.com, this is where add_lvalue_reference<T> differs from T&: https://en.cppreference.com/w/cpp/types/add_reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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 `T &` operator or `const T&` operator");
Copy link
Collaborator

Choose a reason for hiding this comment

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

"with operator `T &` or `const T&`");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

public:
bool load(handle src, bool convert) { return subcaster.load(src, convert); }
static constexpr auto name = caster_t::name;
Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ set(PYBIND11_TEST_FILES
test_constants_and_functions.cpp
test_copy_move.cpp
test_custom_type_casters.cpp
test_chimera.cpp
Copy link
Collaborator

Choose a reason for hiding this comment

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

This name stands out as the only one that is non-descriptive of what is being tested. The name is fitting if the context of the test is known first, but doesn't help people understand what the test is doing without looking inside. Could you please find a descriptive name, in line with the existing test names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

test_docstring_options.cpp
test_eigen.cpp
test_enum.cpp
Expand Down
11 changes: 11 additions & 0 deletions tests/test_builtin_casters.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,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
10 changes: 10 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,15 @@ def test_reference_wrapper():
m.refwrap_usertype(None)
assert "incompatible function arguments" in str(excinfo.value)

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

# const-ness is not propagated.
Copy link
Collaborator

Choose a reason for hiding this comment

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

At second thought, I think it's best to only exercise

assert m.refwrap_lvalue_const().value == 1

and to delete the comment and the other two lines.
Rationale: You want to exercise that the caster changed in this PR is fully covered (load and cast for T& and const T&). The more general issue/feature that const is lost isn't relevant to this purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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

a1 = m.refwrap_list(copy=True)
a2 = m.refwrap_list(copy=True)
assert [x.value for x in a1] == [2, 3]
Expand Down
Loading