Adds automatic casting on assignment of non-pyobject types #551

jagerman · 2016-12-10T06:18:06Z

This adds automatic casting when assigning to python types like dict, list, and attributes. Instead of:

    dict["key"] = py::cast(val);
    m.attr("foo") = py::cast(true);
    list.append(py::cast(42));

you can now simply write:

    dict["key"] = val;
    m.attr("foo") = true;
    list.append(42);

Casts needing extra parameters (e.g. for a non-default rvp) still require the py::cast() call with the appropriate extra arguments.

Fixes #547 (the original post).

This adds automatic casting when assigning to python types like dict, list, and attributes. Instead of: dict["key"] = py::cast(val); m.attr("foo") = py::cast(true); list.append(py::cast(42)); you can now simply write: dict["key"] = val; m.attr("foo") = true; list.append(42); Casts needing extra parameters (e.g. for a non-default rvp) still require the py::cast() call. Fixes pybind#547 (the original post).

dean0x7d

Great, this should make the interface a lot easier to use.

My only concern is that having both the template SFINAE overload and regular handle overload may be too much bookkeeping if/when the C++ API is extended with new methods (list.insert, dict.setdefault and similar). See my comment in the code.

dean0x7d · 2016-12-10T07:14:51Z

docs/basics.rst

-        m.attr("the_answer") = py::cast(42);
-        m.attr("what") = py::cast("World");
+        m.attr("the_answer") = 42;
+        auto world = py::cast("World");


Perhaps use py::object here so readers can get a better idea of what py::cast is doing.

dean0x7d · 2016-12-10T08:16:42Z

include/pybind11/pytypes.h

@@ -774,6 +782,8 @@ class list : public object {
    size_t size() const { return (size_t) PyList_Size(m_ptr); }
    detail::list_accessor operator[](size_t index) const { return {*this, index}; }
    void append(handle h) const { PyList_Append(m_ptr, h.ptr()); }
+    template <typename T, detail::enable_if_t<detail::is_implicitly_castable<T>::value, int> = 0>
+    void append(const T &value) const;


I wonder if this and operator= can be streamlined a bit so that they don't need both the handle and template T overloads? Having just the template overload without the extra SFINAE would be ideal IMO.

I guess the main issue here is that a py::cast on handle would introduce an unnecessary incref/decref, so perhaps there should be something more specialized here. This could be useful for future expansion of the py::object-derived classes. There are lots of methods which could be exposed in C++ similarly to list.append() and it would be unfortunate to have to create 2 overloads for each new method.

jagerman · 2016-12-10T22:34:16Z

The SFINAE is needed, I think, because we want things like py::int_ to go via the py::object overload, which they won't if the template SFINAE isn't there.

I'm open to other suggestions, of course; it is indeed a bit cumbersome (especially because it needs to be duplicated again with the actual implementation in cast.h).

steve-lorimer · 2016-12-11T15:34:33Z

@jagerman thanks, this is great!

dean0x7d · 2016-12-11T16:32:27Z

@jagerman The SFINAE is required, but my main point is that it should be pushed down to py::cast or a new helper function so that it's only done in one place instead of having to repeat it for each interface function. This would simplify the accessor's operator= from 4 to 2 overloads and append from 2 to 1. Similarly new methods would only require 1.

wjakob · 2016-12-11T19:19:27Z

I agree with @dean0x7d: the thought to duplicate all convenience functions in future expansions of various pytypes.h classes seems worrisome. It would be nicer to push the SFINAE into one centralized place.

Combined non-converting handle and autocasting template methods via a helper method that either just returns (handle) or casts (C++ type).

jagerman · 2016-12-12T05:22:29Z

@dean0x7d - take a look at the change I just pushed, I think that is basically what you had in mind, and I definitely agree it's a nicer approach.

wjakob · 2016-12-12T12:29:02Z

include/pybind11/pytypes.h

+// When given a pyobject, this simply returns the pyobject as-is; for other C++ type, the value goes
+// through pybind11::cast(obj) to convert it to an `object`.
+template <typename T, enable_if_t<is_pyobject<T>::value, int> = 0>
+T object_or_cast(T o) { return o; }


Would it make sense to accept an universal reference as T here? In that case, I think that values could be moved into the object_or_cast parameter, avoiding a useless incref/decref.

This overload can do pure forwarding:

auto object_or_cast(T &&o) -> decltype(std::forward<T>(o)) { return std::forward<T>(o); }

Here, the trailing decltype is better than a T return type because it will just do forwarding without generating any copy/move constructors (even if the compiler can optimize them away, why give it extra work).

I've changed all of them to use universal references.

wjakob · 2016-12-12T12:32:06Z

include/pybind11/pytypes.h

+template <typename T, enable_if_t<!is_pyobject<T>::value && !std::is_same<T, PyObject*>::value, int> = 0>
+object object_or_cast(const T &o);
+// Match a PyObject*, which we want to convert directly to handle via its converting constructor
+template <typename T, enable_if_t<std::is_same<T, PyObject>::value, int> = 0> handle object_or_cast(T *ptr) { return ptr; }


It wasn't clear to me why the return value is handle here. Could the reinterpret_borrow be avoided below if we return an object here? Do we even need the PyObject* version?

The reinterpret_borrow is still needed for the previous overload which can also return (forward) a handle.

I don't think the PyObject* overload is needed. The forwarding overload can take care of it.

BTW:

template <typename T, enable_if_t<std::is_same<T, PyObject>::value, int> = 0> handle object_or_cast(T *ptr);

is equivalent to:

inline handle object_or_cast(PyObject *ptr);

I switched the overload to the latter. It's still needed, though (see my comment in the PR).

aldanor · 2016-12-12T12:37:59Z

list::append() was updated, shouldnt set::add() get the same treatment?
There's a few more places in the codebase where explicit casting is still used, like here and here, there's probably a few more (it's mostly casts to int_ and str which is easily searchable).

dean0x7d · 2016-12-12T14:33:28Z

include/pybind11/cast.h

@@ -1120,6 +1120,10 @@ template <> inline void object::cast() && { return; }

 NAMESPACE_BEGIN(detail)

+// Declared in pytypes.h:
+template <typename T, enable_if_t<!is_pyobject<T>::value && !std::is_same<T, PyObject *>::value, int>>
+object object_or_cast(const T &o) { return pybind11::cast(o); }


Should use forwarding: object_or_cast(T &&x) { return pybind11::cast(std::forward<T>(x)); }

dean0x7d · 2016-12-12T14:51:44Z

include/pybind11/pytypes.h

+// When given a pyobject, this simply returns the pyobject as-is; for other C++ type, the value goes
+// through pybind11::cast(obj) to convert it to an `object`.
+template <typename T, enable_if_t<is_pyobject<T>::value, int> = 0>
+T object_or_cast(T o) { return o; }


This overload can do pure forwarding:

auto object_or_cast(T &&o) -> decltype(std::forward<T>(o)) { return std::forward<T>(o); }

Here, the trailing decltype is better than a T return type because it will just do forwarding without generating any copy/move constructors (even if the compiler can optimize them away, why give it extra work).

dean0x7d · 2016-12-12T15:15:17Z

include/pybind11/pytypes.h

+template <typename T, enable_if_t<!is_pyobject<T>::value && !std::is_same<T, PyObject*>::value, int> = 0>
+object object_or_cast(const T &o);
+// Match a PyObject*, which we want to convert directly to handle via its converting constructor
+template <typename T, enable_if_t<std::is_same<T, PyObject>::value, int> = 0> handle object_or_cast(T *ptr) { return ptr; }


The reinterpret_borrow is still needed for the previous overload which can also return (forward) a handle.

I don't think the PyObject* overload is needed. The forwarding overload can take care of it.

BTW:

template <typename T, enable_if_t<std::is_same<T, PyObject>::value, int> = 0> handle object_or_cast(T *ptr);

is equivalent to:

inline handle object_or_cast(PyObject *ptr);

dean0x7d · 2016-12-12T15:23:23Z

include/pybind11/pytypes.h

+    template <typename T, enable_if_t<!std::is_base_of<object, T>::value && !std::is_base_of<accessor, T>::value, int> = 0>
+    void operator=(const T &value) && { Policy::set(obj, key, object_or_cast(value)); }
+    template <typename T, enable_if_t<!std::is_base_of<object, T>::value && !std::is_base_of<accessor, T>::value, int> = 0>
+    void operator=(const T value) & { get_cache() = reinterpret_borrow<object>(object_or_cast(value)); }


I believe the const object & overload can be removed because the template will take care of it. The const accessor & overload must stay in order to override the default assignment operator (templates are not allowed to replace default compiler generated assignments).

The SFINAE can go away because it's handled by normal overload resolution (given an exact match between a template and a non-template function, the non-template function always has priority).

Forward value in the template.

dean0x7d · 2016-12-12T15:26:06Z

include/pybind11/pytypes.h

@@ -773,7 +787,7 @@ class list : public object {
    }
    size_t size() const { return (size_t) PyList_Size(m_ptr); }
    detail::list_accessor operator[](size_t index) const { return {*this, index}; }
-    void append(handle h) const { PyList_Append(m_ptr, h.ptr()); }
+    template <typename T> void append(const T &val) const { PyList_Append(m_ptr, detail::object_or_cast(val).ptr()); }


T&& and forward val.

jagerman · 2016-12-12T18:17:33Z

Updated with the comments.

The PyObject * overload is still needed: without it, the casting template matches a PyObject * (which is used in many places), but that's wrong (it goes to py::cast(obj) which isn't valid for a PyObject *). In the previous, non-templated code, providing a PyObject * worked by implicit conversion to handle via handle's implicit conversion constructor. With the templated version, even with an explicit handle overload, that doesn't work anymore: the non-pyobject templated method takes precedence since provides an exact match while the handle overload only matches via an implicit conversion.

jagerman · 2016-12-12T18:34:30Z

list::append() was updated, shouldnt set::add() get the same treatment?

Indeed, fixed.

There's a few more places in the codebase where explicit casting is still used, like here and here, there's probably a few more (it's mostly casts to int_ and str which is easily searchable).

Do you think it's worth changing them all? There's no real advantage to implicit casting other than coding convenience, but existing code is already written (and keeping at least some of it, particularly in the test code, helps ensure that it still works as expected). The point of this wasn't really to eliminate explicit casting, but just to allow implicit casting as an alternative in common cases.

wjakob · 2016-12-12T22:41:31Z

I think this looks great now -- thank you! I'll go ahead and merge it.

jagerman mentioned this pull request Dec 10, 2016

Unable to store std::string in py::dict #547

Closed

jagerman force-pushed the implicit-accessor-casting branch from ddc4868 to fb26eac Compare December 10, 2016 06:28

dean0x7d reviewed Dec 10, 2016

View reviewed changes

Use actual type, not auto, in basic documentation for clarity

2b69a74

Simplify accessor implicit casting

f064055

Combined non-converting handle and autocasting template methods via a helper method that either just returns (handle) or casts (C++ type).

jagerman added 2 commits December 12, 2016 00:27

Remove unwanted default template arg from definition

8683f76

Take =/append values by const lvalue ref

659daad

wjakob reviewed Dec 12, 2016

View reviewed changes

dean0x7d reviewed Dec 12, 2016

View reviewed changes

Forward values, simplify templates, support set.add()

eccc596

wjakob merged commit 3f1ff3f into pybind:master Dec 12, 2016

rwgk mentioned this pull request Feb 9, 2023

FWD pybind11 google/pybind11clif#551

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds automatic casting on assignment of non-pyobject types #551

Adds automatic casting on assignment of non-pyobject types #551

jagerman commented Dec 10, 2016

dean0x7d left a comment

dean0x7d Dec 10, 2016

dean0x7d Dec 10, 2016 •

edited

Loading

jagerman commented Dec 10, 2016

steve-lorimer commented Dec 11, 2016

dean0x7d commented Dec 11, 2016

wjakob commented Dec 11, 2016

jagerman commented Dec 12, 2016

wjakob Dec 12, 2016

dean0x7d Dec 12, 2016

jagerman Dec 12, 2016

wjakob Dec 12, 2016

dean0x7d Dec 12, 2016

jagerman Dec 12, 2016

aldanor commented Dec 12, 2016

dean0x7d Dec 12, 2016

dean0x7d Dec 12, 2016

dean0x7d Dec 12, 2016

dean0x7d Dec 12, 2016

dean0x7d Dec 12, 2016

jagerman commented Dec 12, 2016

jagerman commented Dec 12, 2016

wjakob commented Dec 12, 2016

Adds automatic casting on assignment of non-pyobject types #551

Adds automatic casting on assignment of non-pyobject types #551

Conversation

jagerman commented Dec 10, 2016

dean0x7d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dean0x7d Dec 10, 2016 • edited Loading

Choose a reason for hiding this comment

jagerman commented Dec 10, 2016

steve-lorimer commented Dec 11, 2016

dean0x7d commented Dec 11, 2016

wjakob commented Dec 11, 2016

jagerman commented Dec 12, 2016

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

aldanor commented Dec 12, 2016

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 Dec 12, 2016

jagerman commented Dec 12, 2016

wjakob commented Dec 12, 2016

dean0x7d Dec 10, 2016 •

edited

Loading