Add movable cast support to type casters #851
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Looks good to me! |
dean0x7d
reviewed
May 22, 2017
include/pybind11/cast.h
Outdated
| @@ -374,11 +374,31 @@ class type_caster_generic { | |||
| object temp; | |||
| }; | |||
|
|
|||
| /* Determine suitable casting operator */ | |||
| /** Determine suitable casting operator for pointer-or-lvalue-casting type casters. The type caster | |||
| * needs to provide `operator T*()` and `operator T&()` operators. | |||
There was a problem hiding this comment.
Style nitpick: could the first and second text line be aligned? I.e.:
/**
* Determine suitable casting operator for pointer-or-lvalue-casting type casters. The type caster
* needs to provide `operator T*()` and `operator T&()` operators.
*/(The misalignment probably bothers me more than it should.)
There was a problem hiding this comment.
Yeah, that does look better. 37b2383 does this on master, and I'll update this PR.
This commit allows type_casters to allow their local values to be moved away, rather than copied, when the type caster instance itself is an rvalue. This only applies (automatically) to type casters using PYBIND11_TYPE_CASTER; the generic type type casters don't own their own pointer, and various value casters (e.g. std::string, std::pair, arithmetic types) already cast to an rvalue (i.e. they return by value). This updates various calling code to attempt to get a movable value whenever the value is itself coming from a type caster about to be destroyed: for example, when constructing an std::pair or various stl.h containers. For types that don't support value moving, the cast_op falls back to an lvalue cast. There wasn't an obvious place to add the tests, so I added them to test_copy_move_policies, but also renamed it to drop the _policies as it now tests more than just policies.
jagerman
force-pushed
the
movable-cast-ops
branch
from
e328bd6
to
5a0e8ac
Compare
|
This looks great! The 4KB change is definitely above the noise floor :) |
bmerry
added a commit
to bmerry/pybind11
that referenced
this pull request
May 26, 2017
Now that pybind#851 has removed all multiple uses of a caster, it can just use the default-constructed value with needing a reset. This fixes two issues: 1. With std::experimental::optional (at least under GCC 5.4), the `= {}` would construct an instance of the optional type and then move-assign it, which fails if the value type isn't move-assignable. 2. With older versions of Boost, the `= {}` could fail because it is ambiguous, allowing construction of either `boost::none` or the value type.
dean0x7d
pushed a commit
that referenced
this pull request
May 27, 2017
Now that #851 has removed all multiple uses of a caster, it can just use the default-constructed value with needing a reset. This fixes two issues: 1. With std::experimental::optional (at least under GCC 5.4), the `= {}` would construct an instance of the optional type and then move-assign it, which fails if the value type isn't move-assignable. 2. With older versions of Boost, the `= {}` could fail because it is ambiguous, allowing construction of either `boost::none` or the value type.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit allows type casters to allow their local values to be moved away, rather than copied, when the type caster instance itself is an rvalue.
This only applies (automatically) to type casters using
PYBIND11_TYPE_CASTER; the generic type type casters don't own their own pointer, and various value casters (e.g.std::string,std::pair, arithmetic types) already cast to an rvalue (i.e. their cast op is a return-by-value).This updates various calling code to attempt to get a movable value whenever the value is itself coming from a type caster about to be destroyed: for example, when constructing a
std::pairor variousstl.hcontainers. For types that don't support value moving, these casts fall back to an lvalue cast.There wasn't an obvious place to add the tests, so I added them to test_copy_move_policies, but also renamed it to drop the _policies as it now tests more than just policies.
.so size (before adding the tests) is slightly reduced, though the reduction is small enough (just under 4K) that it could just be noise; suffice it to say, there is no significant increase.