-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Adjusting type_caster<std::reference_wrapper<T>>
to support const/non-const propagation in cast_op
.
#2705
Changes from 24 commits
65abb62
c603907
5cdc0ca
35e7fa5
4cd06d3
5abd823
c5bd104
f0e41f5
b56a568
d71cd7a
3c69635
8f32ba3
d144e59
a7ec72e
e1aca4b
005d3fe
4ac23ca
a3d9b18
5b4fb98
8dcbc42
9f77862
7672e92
c747ae2
33f57cc
d869976
f109edd
775a7c9
6f6dac7
fd6dc5b
b3ecc53
c22772b
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 | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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; | ||||||||||||||||||||
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. Why not just 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. To be more specific, According to cppreference.com, this is where 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. Done. |
||||||||||||||||||||
using subcaster_cast_op_type = | ||||||||||||||||||||
laramiel marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
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 `const T&`"); | ||||||||||||||||||||
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. Since this line isn't as long as this one below:
It's probably more clear if we put it like this? (also with correct indentation and with
Suggested change
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. Done. |
||||||||||||||||||||
public: | ||||||||||||||||||||
bool load(handle src, bool convert) { return subcaster.load(src, convert); } | ||||||||||||||||||||
static constexpr auto name = caster_t::name; | ||||||||||||||||||||
|
@@ -973,7 +979,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 subcaster.operator subcaster_cast_op_type(); } | ||||||||||||||||||||
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.
Suggested change
or even (depending on the other discussion)
Suggested change
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. So you already listed point 3/ where I removed a & based on review feedback as a "separate change"; how is this not the same? 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. What do you mean? I don't mean this should be taken out, but if you go and change 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. If you don't agree with the suggestion in #2705 (comment), that's also fine with me; then just argue why not and do not do it! 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 mean 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. True, yes. I just saw this being used in other casters. 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'm also fine with not applying this change; it's supposed to do exactly the same, though. 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. Done. |
||||||||||||||||||||
}; | ||||||||||||||||||||
|
||||||||||||||||||||
#define PYBIND11_TYPE_CASTER(type, py_name) \ | ||||||||||||||||||||
|
Uh oh!
There was an error while loading. Please reload this page.