-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix casters of STL containers with char*
#2303
base: master
Are you sure you want to change the base?
Fix casters of STL containers with char*
#2303
Conversation
@@ -342,6 +369,7 @@ struct variant_caster<V<Ts...>> { | |||
auto caster = make_caster<U>(); | |||
if (caster.load(src, convert)) { | |||
value = cast_op<U>(caster); | |||
subcaster = std::move(caster); |
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.
Actually, are casters movable?
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.
They are.
An alterantive to solve #2245 would be to just disallow |
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.
This does look really expensive from here. Like we discussed, it would also need tests and there's a problem of memory and types like vector<vector<vector<...<T>>>>
.
@@ -287,6 +311,9 @@ template<typename T> struct optional_caster { | |||
} | |||
|
|||
PYBIND11_TYPE_CASTER(T, _("Optional[") + value_conv::name + _("]")); | |||
|
|||
private: | |||
value_conv inner_caster; |
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.
Call it subcaster
, for consistency's sake.
@@ -342,6 +369,7 @@ struct variant_caster<V<Ts...>> { | |||
auto caster = make_caster<U>(); | |||
if (caster.load(src, convert)) { | |||
value = cast_op<U>(caster); | |||
subcaster = std::move(caster); |
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.
They are.
OK, this still has a major problem, actually. For types that aren't movable or aren't empty when moved out (like So |
I think the following things are at play here:
I don't think you can get around "all the things need to stay alive", at least for the above cases. The only two solutions I see:
Do you imagine other possibilities? |
Er, also, is it linear or exponential? I think it may be exponential, like |
And looking at @kenoslund-google's overview in #2527, yeah, I think that's what they're implying -- the specialization approach. |
This cóuld fix #2245, I believe. Would you mind testing, if it does, @codypiersall?
If it does, the question is if this is a price we're willing to pay for it?
On the one hand, the casters for
tuple
,pair
, etc, also hold on to their subcasters. On the other hand, it might be a heavy price to "just" solve the issue withchar *
? Maybe there is a better way to keep the thingschar*
point to alive?Closes #2245 (if we'd decide to merge it).