From 373da824869863459e0851c3ad5564073bf83d10 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 3 Aug 2017 19:27:04 -0400 Subject: [PATCH] Make PYBIND11_OBJECT_CVT only convert if the type check fails Currently types that are capable of conversion always call their convert function when invoked with a `py::object` which is actually the correct type. This means that code such as `py::cast(obj)` and `py::list l(obj.attr("list"))` make copies, which was an oversight rather than an intentional feature. While at first glance there might be something behind having `py::list(obj)` make a copy (as it would in Python), this would be inconsistent when you dig a little deeper because `py::list(l)` *doesn't* make a copy for an existing `py::list l`, and having an inconsistency within C++ would be worse than a C++ <-> Python inconsistency. It is possible to get around the copying using a `reinterpret_borrow(o)` (and this commit fixes one place, in `embed.h`, that does so), but that seems a misuse of `reinterpret_borrow`, which is really supposed to be just for dealing with raw python-returned values, not `py::object`-derived wrappers which are supposed to be higher level. This changes the constructor of such converting types (i.e. anything using PYBIND11_OBJECT_CVT -- `str`, `bool_`, `int_`, `float_`, `tuple`, `dict`, `list`, `set`, `memoryview`) to reference rather than copy when the check function passes. It also adds an `object &&` constructor that is slightly more efficient by avoiding an inc_ref when the check function passes. --- include/pybind11/embed.h | 3 +-- include/pybind11/pytypes.h | 7 ++++++- tests/test_pytypes.py | 11 +++++++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h index 0eb656b0..98bf7690 100644 --- a/include/pybind11/embed.h +++ b/include/pybind11/embed.h @@ -99,8 +99,7 @@ inline void initialize_interpreter(bool init_signal_handlers = true) { Py_InitializeEx(init_signal_handlers ? 1 : 0); // Make .py files in the working directory available by default - auto sys_path = reinterpret_borrow(module::import("sys").attr("path")); - sys_path.append("."); + module::import("sys").attr("path").cast().append("."); } /** \rst diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 095d40f1..b652b8e9 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -731,7 +731,12 @@ NAMESPACE_END(detail) #define PYBIND11_OBJECT_CVT(Name, Parent, CheckFun, ConvertFun) \ PYBIND11_OBJECT_COMMON(Name, Parent, CheckFun) \ /* This is deliberately not 'explicit' to allow implicit conversion from object: */ \ - Name(const object &o) : Parent(ConvertFun(o.ptr()), stolen_t{}) { if (!m_ptr) throw error_already_set(); } + Name(const object &o) \ + : Parent(check_(o) ? o.inc_ref().ptr() : ConvertFun(o.ptr()), stolen_t{}) \ + { if (!m_ptr) throw error_already_set(); } \ + Name(object &&o) \ + : Parent(check_(o) ? o.release().ptr() : ConvertFun(o.ptr()), stolen_t{}) \ + { if (!m_ptr) throw error_already_set(); } #define PYBIND11_OBJECT(Name, Parent, CheckFun) \ PYBIND11_OBJECT_COMMON(Name, Parent, CheckFun) \ diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index 1c75c47d..b8dad841 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -174,9 +174,20 @@ def test_constructors(): } inputs = {k.__name__: v for k, v in data.items()} expected = {k.__name__: k(v) for k, v in data.items()} + assert m.converting_constructors(inputs) == expected assert m.cast_functions(inputs) == expected + # Converting constructors and cast functions should just reference rather + # than copy when no conversion is needed: + noconv1 = m.converting_constructors(expected) + for k in noconv1: + assert noconv1[k] is expected[k] + + noconv2 = m.cast_functions(expected) + for k in noconv2: + assert noconv2[k] is expected[k] + def test_implicit_casting(): """Tests implicit casting when assigning or appending to dicts and lists."""