tuple/pair/stl.h casters: use rvalue subcasting when casting an rvalue tuple/pair/container #936

jagerman · 2017-07-03T23:16:05Z

The tuple caster had only a const-lvalue-ref cast method, which meant
that subcaster casts couldn't distinguish between an lvalue and rvalue
cast; this caused Eigen values in a tuple, for instance, to end up with
a readonly flag.

This fixes it by adding an rvalue versions of cast() and cast_impl()
that move the tuple values when subcasting them.

Fixes #935 and #853

Edit: I've updated this PR to also consolidate the tuple/pair casters into a single tuple_caster implementation. It is essentially just the std::tuple implementation, which works perfectly well with std::pairs. Thus the fix here also gets applied to std::pair.

Edit 2: Also stl.h casters get the same treatment, which (incidentally) also fixes #853.

jagerman · 2017-07-04T19:34:05Z

I've updated this PR to also consolidate the tuple/pair casters into a single tuple_caster implementation. It is essentially just the std::tuple implementation, which works perfectly well with std::pairs. Thus the fix here also gets applied to std::pair.

I've also added tests.

wjakob · 2017-07-04T21:19:20Z

This looks great! Combining the pair and tuple casters is a nice simplification.

YannickJadoul · 2017-07-04T21:21:10Z

I'm curious: this reminds me of/sounds like it is very similar to the problem of returning an vector of move-only types (cfr. #853). Could this kind of solution also help there, for list_caster ?

jagerman · 2017-07-04T21:21:47Z

I'm working on doing the same for stl.h right now; will update this PR with it shortly.

YannickJadoul · 2017-07-04T21:22:54Z

Awesome, thanks! :-)

jagerman · 2017-07-05T01:15:27Z

@YannickJadoul - This also fixes #853 now (I used an adapted version your test case in the tests, which now passes).

dean0x7d

The consolidation of pair/tuple is very nice! Looks great overall. The only significant suggestion I have is adding a forward_like<Container>() function (see comments).

dean0x7d · 2017-07-05T09:59:02Z

include/pybind11/cast.h

@@ -1338,14 +1304,15 @@ template <typename... Tuple> class type_caster<std::tuple<Tuple...>> {
        return true;
    }

-    static handle cast_impl(const type &, return_value_policy, handle,
+    template <typename T>
+    static handle cast_impl(T &&, return_value_policy, handle,


Since it's empty, it should be fine to leave this one as const types & and avoid a template instantiation.

With just the const type & an rvalue argument will end up at the full template version. I suppose I could put in dual empty methods (one lvalue, one rvalue) if you like that better (though there are still things like non-const lvalue and const rvalue that would slip by).

Right, I missed that. Never mind, it's better to leave it with T && then.

I'll try just taking it out completely (i.e. so it lands in the base case) and see if that causes any problems.

Do you prefer keeping it in (and templated), or taking it out entirely, as in the last commit?

👍 for removing it completely -- perfect!

dean0x7d · 2017-07-05T10:00:30Z

include/pybind11/cast.h

@@ -1312,6 +1275,10 @@ template <typename... Tuple> class type_caster<std::tuple<Tuple...>> {
        return cast_impl(src, policy, parent, indices{});
    }

+    static handle cast(type &&src, return_value_policy policy, handle parent) {


Maybe merge the const& and && overloads into a single function with forwarding?

dean0x7d · 2017-07-05T12:14:12Z

include/pybind11/stl.h

+template <typename ContainerArg, typename Value>
+using forward_item = conditional_t<
+    std::is_lvalue_reference<ContainerArg>::value, const Value &,
+    typename std::add_rvalue_reference<Value>::type>;


Explicitly using const Value & is a bit less generic than it could be: remove_reference_t<Value>& and remove_reference_t<Value>&&.

Consider wrapping it in a forward_like function like here: https://stackoverflow.com/a/29780197/7189436
forward_like<Container>(element) is pretty nice and expressive.

dean0x7d · 2017-07-05T12:34:23Z

include/pybind11/cast.h

-    using indices = make_index_sequence<sizeof...(Tuple)>;
+// Base implementation for std::tuple and std::pair
+template <template<typename...> class TupleType, typename... Tuple> class tuple_caster {
+    using type = TupleType<Tuple...>;


Consider renaming Tuple... into Ts... or similar. At first glance TupleType<Tuple...> looks like a tuple of tuples.

dean0x7d · 2017-07-05T12:44:41Z

tests/test_stl.cpp

@@ -61,6 +61,50 @@ TEST_SUBMODULE(stl, m) {
        return set.count("key1") && set.count("key2") && set.count("key3");
    });

+    #if defined(_MSC_VER) && _MSC_VER < 1910
+    // We get some really long type names here which causes MSVC 2015 to emit warnings
+    #  pragma warning(disable: 4503) // warning C4503: decorated name length exceeded, name was truncated


I'd suggest adding this globally to pybind11_tests.h since it's already required in two files and likely more in the future.

jagerman · 2017-07-05T13:52:31Z

adding a forward_like<Container>() function

That's a nice suggestion, and it shortens up the code a bit too (by eliminating the using Forward = ...). I kept it in stl.h for now; we can move it to the block of utility templates/functions in common.h if/when something else wants it.

The std::pair caster can be written as a special case of the std::tuple caster; this combines them via a base `tuple_caster` class (which is essentially identical to the previous std::tuple caster). This also removes the special empty tuple base case: returning an empty tuple is relatively rare, and the base case still works perfectly well even when the tuple types is an empty list.

This updates the std::tuple, std::pair and `stl.h` type casters to forward their contained value according to whether the container being cast is an lvalue or rvalue reference. This fixes an issue where subcaster casts were always called with a const lvalue which meant nested type casters didn't have the desired `cast()` overload invoked. For example, this caused Eigen values in a tuple to end up with a readonly flag (issue pybind#935) and made it impossible to return a container of move-only types (issue pybind#853). This fixes both issues by adding templated universal reference `cast()` methods to the various container types that forward container elements according to the container reference type.

PR pybind#936 broke the ability to return a pointer to a stl container (and, likewise, to a tuple) because the added deduced type matched a non-const pointer argument: the pointer-accepting `cast` in PYBIND11_TYPE_CASTER had a `const type *`, which is a worse match for a non-const pointer than the universal reference template pybind#936 added. This changes the provided TYPE_CASTER cast(ptr) to take the pointer by template arg (so that it will accept either const or non-const pointer). It has two other effects: it slightly reduces .so size (because many type casters never actually need the pointer cast at all), and it allows type casters to provide their untemplated pointer `cast()` that will take precedence over the templated version provided in the macro.

PR #936 broke the ability to return a pointer to a stl container (and, likewise, to a tuple) because the added deduced type matched a non-const pointer argument: the pointer-accepting `cast` in PYBIND11_TYPE_CASTER had a `const type *`, which is a worse match for a non-const pointer than the universal reference template #936 added. This changes the provided TYPE_CASTER cast(ptr) to take the pointer by template arg (so that it will accept either const or non-const pointer). It has two other effects: it slightly reduces .so size (because many type casters never actually need the pointer cast at all), and it allows type casters to provide their untemplated pointer `cast()` that will take precedence over the templated version provided in the macro.

jagerman mentioned this pull request Jul 3, 2017

scipy.optimize.fmin_l_bfgs_b thinks that my array isn't fortran contiguous #935

Closed

jagerman force-pushed the tuple-rvalue-casts branch 2 times, most recently from ae91eca to 79d5baf Compare July 4, 2017 19:31

jagerman changed the title ~~WIP: std::tuple caster: use rvalue subcasting when casting an rvalue tuple~~ tuple/pair caster: use rvalue subcasting when casting an rvalue tuple/pair Jul 4, 2017

dean0x7d reviewed Jul 5, 2017

View reviewed changes

jagerman added 2 commits July 5, 2017 12:17

jagerman force-pushed the tuple-rvalue-casts branch from c0a3f20 to 3498e79 Compare July 5, 2017 16:25

jagerman changed the title ~~tuple/pair caster: use rvalue subcasting when casting an rvalue tuple/pair~~ tuple/pair/stl.h casters: use rvalue subcasting when casting an rvalue tuple/pair/container Jul 5, 2017

jagerman merged commit b57281b into pybind:master Jul 5, 2017

jagerman mentioned this pull request Jul 7, 2017

Custom type caster pointer ownership support (and container regression fix) #938

Merged

dean0x7d modified the milestone: v2.2 Aug 13, 2017

jagerman deleted the tuple-rvalue-casts branch August 14, 2017 20:26

rwgk mentioned this pull request Feb 9, 2023

FWD pybind11 google/pybind11clif#936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tuple/pair/stl.h casters: use rvalue subcasting when casting an rvalue tuple/pair/container #936

tuple/pair/stl.h casters: use rvalue subcasting when casting an rvalue tuple/pair/container #936

jagerman commented Jul 3, 2017 •

edited

Loading

jagerman commented Jul 4, 2017

wjakob commented Jul 4, 2017

YannickJadoul commented Jul 4, 2017

jagerman commented Jul 4, 2017

YannickJadoul commented Jul 4, 2017

jagerman commented Jul 5, 2017

dean0x7d left a comment •

edited

Loading

dean0x7d Jul 5, 2017

jagerman Jul 5, 2017

dean0x7d Jul 5, 2017

jagerman Jul 5, 2017

jagerman Jul 5, 2017

dean0x7d Jul 5, 2017

dean0x7d Jul 5, 2017

dean0x7d Jul 5, 2017

dean0x7d Jul 5, 2017

dean0x7d Jul 5, 2017

jagerman commented Jul 5, 2017

tuple/pair/stl.h casters: use rvalue subcasting when casting an rvalue tuple/pair/container #936

tuple/pair/stl.h casters: use rvalue subcasting when casting an rvalue tuple/pair/container #936

Conversation

jagerman commented Jul 3, 2017 • edited Loading

jagerman commented Jul 4, 2017

wjakob commented Jul 4, 2017

YannickJadoul commented Jul 4, 2017

jagerman commented Jul 4, 2017

YannickJadoul commented Jul 4, 2017

jagerman commented Jul 5, 2017

dean0x7d left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagerman commented Jul 5, 2017

jagerman commented Jul 3, 2017 •

edited

Loading

dean0x7d left a comment •

edited

Loading