From 65abb621b2afa0694cbb4f861303a790e6a847d8 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Mon, 30 Nov 2020 09:43:38 -0800 Subject: [PATCH 01/31] Allow type_caster of std::reference_wrapper to be the same as a native reference. Before, both std::reference_wrapper and std::reference_wrapper would invoke cast_op. This doesn't allow the type_caster<> specialization for T to distinguish reference_wrapper types from value types. After, the type_caster<> specialization invokes cast_op, which allows reference_wrapper to behave in the same way as a native reference type. --- include/pybind11/cast.h | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index c428e3f13a..7a13debdfa 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -958,11 +958,18 @@ cast_op(make_caster &&caster) { template class type_caster> { private: - using caster_t = make_caster; - caster_t subcaster; - using subcaster_cast_op_type = typename caster_t::template cast_op_type; - static_assert(std::is_same::type &, subcaster_cast_op_type>::value, - "std::reference_wrapper caster requires T to have a caster with an `T &` operator"); + using reference_t = typename std::add_lvalue_reference::type; + using caster_t = make_caster; + caster_t subcaster; + using subcaster_cast_op_type = + typename caster_t::template cast_op_type; + + static_assert((std::is_same::type &, + subcaster_cast_op_type>::value || + std::is_same::value), + "std::reference_wrapper caster requires T to have a caster " + "with an `T &` operator or `const T&` operator"); + public: bool load(handle src, bool convert) { return subcaster.load(src, convert); } static constexpr auto name = caster_t::name; From c6039075d9f665e8b1b5a61adf7cd44df49df046 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Mon, 30 Nov 2020 10:29:30 -0800 Subject: [PATCH 02/31] Add tests/examples for std::reference_wrapper --- include/pybind11/cast.h | 13 ++++++------- tests/test_builtin_casters.cpp | 11 +++++++++++ tests/test_builtin_casters.py | 10 ++++++++++ 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 7a13debdfa..39f065ce92 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -958,18 +958,17 @@ cast_op(make_caster &&caster) { template class type_caster> { private: - using reference_t = typename std::add_lvalue_reference::type; - using caster_t = make_caster; - caster_t subcaster; - using subcaster_cast_op_type = - typename caster_t::template cast_op_type; + using caster_t = make_caster; + caster_t subcaster; + using reference_t = typename std::add_lvalue_reference::type; + using subcaster_cast_op_type = + typename caster_t::template cast_op_type; - static_assert((std::is_same::type &, + static_assert((std::is_same::type &, subcaster_cast_op_type>::value || std::is_same::value), "std::reference_wrapper caster requires T to have a caster " "with an `T &` operator or `const T&` operator"); - public: bool load(handle src, bool convert) { return subcaster.load(src, convert); } static constexpr auto name = caster_t::name; diff --git a/tests/test_builtin_casters.cpp b/tests/test_builtin_casters.cpp index acc9f8fb36..8fd5b73199 100644 --- a/tests/test_builtin_casters.cpp +++ b/tests/test_builtin_casters.cpp @@ -147,6 +147,17 @@ TEST_SUBMODULE(builtin_casters, m) { // test_reference_wrapper m.def("refwrap_builtin", [](std::reference_wrapper p) { return 10 * p.get(); }); m.def("refwrap_usertype", [](std::reference_wrapper p) { return p.get().value(); }); + m.def("refwrap_usertype_const", [](std::reference_wrapper p) { return p.get().value(); }); + + m.def("refwrap_lvalue", []() -> std::reference_wrapper { + static UserType x(1); + return std::ref(x); + }); + m.def("refwrap_lvalue_const", []() -> std::reference_wrapper { + static UserType x(1); + return std::cref(x); + }); + // Not currently supported (std::pair caster has return-by-value cast operator); // triggers static_assert failure. //m.def("refwrap_pair", [](std::reference_wrapper>) { }); diff --git a/tests/test_builtin_casters.py b/tests/test_builtin_casters.py index bd7996b620..1338909595 100644 --- a/tests/test_builtin_casters.py +++ b/tests/test_builtin_casters.py @@ -315,6 +315,7 @@ def test_reference_wrapper(): """std::reference_wrapper for builtin and user types""" assert m.refwrap_builtin(42) == 420 assert m.refwrap_usertype(UserType(42)) == 42 + assert m.refwrap_usertype_const(UserType(42)) == 42 with pytest.raises(TypeError) as excinfo: m.refwrap_builtin(None) @@ -324,6 +325,15 @@ def test_reference_wrapper(): m.refwrap_usertype(None) assert "incompatible function arguments" in str(excinfo.value) + assert m.refwrap_lvalue_const().value == 1 + # const-ness is not propagated. + # m.refwrap_lvalue_const().value =2 + # assert m.refwrap_lvalue_const().value == 1 + + assert m.refwrap_lvalue().value == 1 + m.refwrap_lvalue().value = 4 + assert m.refwrap_lvalue().value == 4 + a1 = m.refwrap_list(copy=True) a2 = m.refwrap_list(copy=True) assert [x.value for x in a1] == [2, 3] From 5cdc0caf9053b6b2b2f86f5058d1c759a8355540 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Tue, 1 Dec 2020 19:16:27 -0800 Subject: [PATCH 03/31] Add tests which use mutable/immutable variants This test is a chimera; it blends the pybind11 casters with a custom pytype implementation that supports immutable and mutable calls. In order to detect the immutable/mutable state, the cast_op needs to propagate it, even through e.g. std::reference Note: This is still a work in progress; some things are crashing, which likely means that I have a refcounting bug or something else missing. --- tests/CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index dae8b5ad43..04cce5c45b 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -96,6 +96,7 @@ set(PYBIND11_TEST_FILES test_constants_and_functions.cpp test_copy_move.cpp test_custom_type_casters.cpp + test_chimera.cpp test_docstring_options.cpp test_eigen.cpp test_enum.cpp From 35e7fa557b5501979936b8180af70874c4814519 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Tue, 1 Dec 2020 20:33:27 -0800 Subject: [PATCH 04/31] Add/finish tests that distinguish const& from & Fixes the bugs in my custom python type implementation, demonstrate test that requires const& and reference_wrapper being treated differently from Non-const. --- tests/test_chimera.cpp | 289 +++++++++++++++++++++++++++++++++++++++++ tests/test_chimera.py | 69 ++++++++++ 2 files changed, 358 insertions(+) create mode 100644 tests/test_chimera.cpp create mode 100644 tests/test_chimera.py diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp new file mode 100644 index 0000000000..20a6e85915 --- /dev/null +++ b/tests/test_chimera.cpp @@ -0,0 +1,289 @@ +/* + tests/test_chimera.cpp -- This demonstrates a hybrid usage of pybind11, where the + type caster returns a hand-rolled python object type rather than relying on the + natural python bindings. + + All rights reserved. Use of this source code is governed by a + BSD-style license that can be found in the LICENSE file. +*/ +#include +#include + +#include +#include +#include +#include + +#include "pybind11_tests.h" + +/// C++ type +class Chimera { + public: + int64_t x = 1; +}; + +/// Python wrapper for C++ type which supports mutable/immutable variants. +typedef struct PyChimera { + PyObject_HEAD; + + Chimera* value; + + bool is_immutable; + bool is_owned; +} PyChimera; + +PyObject* PyChimera_getattro(PyObject* self, PyObject* name) { + PyChimera* custom = reinterpret_cast(self); + assert(custom != nullptr); + + const char* attr = nullptr; + if (PyBytes_Check(name)) { + attr = PyBytes_AsString(name); + } else if (PyUnicode_Check(name)) { + attr = PyUnicode_AsUTF8(name); + } + if (std::string_view(attr) == "x") { + return PyLong_FromLong(custom->value->x); + } + + return PyObject_GenericGetAttr(self, name); +} + +int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { + PyChimera* custom = reinterpret_cast(self); + assert(custom != nullptr); + + const char* attr = nullptr; + if (PyBytes_Check(name)) { + attr = PyBytes_AsString(name); + } else if (PyUnicode_Check(name)) { + attr = PyUnicode_AsUTF8(name); + } + if (std::string_view(attr) == "x") { + if (!PyLong_Check(value)) { + PyErr_Format(PyExc_ValueError, "Cannot set a non-numeric value"); + return -1; + } + if (custom->is_immutable) { + PyErr_Format(PyExc_ValueError, "Instance is immutable; cannot set values"); + return -1; + } + custom->value->x = static_cast(PyLong_AsLong(value)); + return 0; + } + + return PyObject_GenericSetAttr(self, name, value); +} + +void PyChimera_dealloc(PyObject* self); + +static PyTypeObject PyChimera_Type = { + .ob_base = PyVarObject_HEAD_INIT(nullptr, 0) /**/ + .tp_name = + "google3.experimental.users.lar.python.test_custom.Chimera", + .tp_basicsize = sizeof(PyChimera), + .tp_dealloc = &PyChimera_dealloc, + .tp_getattro = &PyChimera_getattro, + .tp_setattro = &PyChimera_setattro, + .tp_flags = Py_TPFLAGS_DEFAULT, + .tp_doc = "Chimera objects", +}; + +static std::unordered_map* mapping = + new std::unordered_map(); + +void PyChimera_dealloc(PyObject* self) { + PyChimera* custom = reinterpret_cast(self); + auto it = mapping->find(custom->value); + if (it != mapping->end()) { + mapping->erase(it); + } + if (custom->is_owned) { + delete custom->value; + } + Py_TYPE(self)->tp_free(self); +} + +PyObject* PyChimera_new(Chimera* value, bool is_owned, bool is_immutable) { + if (PyType_Ready(&PyChimera_Type) != 0) { + return nullptr; + } + PyChimera* self = PyObject_New(PyChimera, &PyChimera_Type); + if (!self) { + return nullptr; + } + mapping->emplace(value, self); + self->value = value; + self->is_owned = is_owned; + self->is_immutable = is_immutable; + return reinterpret_cast(self); +} + +PyObject* PyChimera_reference(Chimera* value, bool is_immutable) { + auto it = mapping->find(value); + if (it != mapping->end()) { + PyChimera* self = static_cast(it->second); + if (!is_immutable) { + self->is_immutable = false; + } + Py_INCREF(self); + return reinterpret_cast(self); + } + return PyChimera_new(value, false, is_immutable); +} + +/// pybind11 typecaster which returns python wrapper type. + +namespace pybind11 { +namespace detail { + +template <> +struct type_caster { + static constexpr auto name = _(); + + // C++ -> Python + // ... construct an immutable python type referencing src. This isn't really + // available in pybind11 and would have to be built by hand. + static handle cast(const Chimera* src, return_value_policy, handle) { + if (!src) return pybind11::none().release(); + return PyChimera_reference(const_cast(src), true); + } + static handle cast(const Chimera& src, return_value_policy policy, + handle parent) { + return cast(&src, policy, parent); + } + + // ... construct a mutable python type referencing src. This is the default + // pybind11 path. + static handle cast(Chimera* src, return_value_policy, handle) { + if (!src) return pybind11::none().release(); + return PyChimera_reference(src, false); + } + static handle cast(Chimera& src, return_value_policy policy, handle parent) { + return cast(&src, policy, parent); + } + + // construct a mutable python type owning src. + static handle cast(Chimera&& src, return_value_policy, handle) { + return PyChimera_new(new Chimera(std::move(src)), true, false); + } + + // Convert Python->C++. + bool load(handle src, bool) { + // ... either reference a wrapped c++ type or construct a new one. + if (!PyObject_TypeCheck(src.ptr(), &PyChimera_Type)) { + return false; + } + custom = reinterpret_cast(src.ptr()); + return true; + } + + // cast_op_type determines which operator overload to call for a given c++ + // input parameter type. In this case we want to propagate const, etc. + template + using cast_op_type = conditional_t< + std::is_same, const Chimera*>::value, const Chimera*, + conditional_t< + std::is_same, Chimera*>::value, Chimera*, + conditional_t::value, const Chimera&, + conditional_t::value, Chimera&, + /*default is T&&*/ T_>>>>; + + // PYBIND11_TYPE_CASTER + operator const Chimera*() { return custom->value; } + operator const Chimera&() { + if (!custom || !custom->value) throw reference_cast_error(); + return *custom->value; + } + operator Chimera*() { + if (custom->is_immutable) throw reference_cast_error(); + return custom->value; + } + operator Chimera&() { + if (!custom || !custom->value) throw reference_cast_error(); + if (custom->is_immutable) throw reference_cast_error(); + return *custom->value; + } + operator Chimera&&() && { + if (!custom || !custom->value) throw reference_cast_error(); + owned = *custom->value; + return std::move(owned); + } + + protected: + const PyChimera* custom; + Chimera owned; +}; + +} // namespace detail +} // namespace pybind11 + +static Chimera* shared = new Chimera(); +static Chimera* shared_const = new Chimera(); + +TEST_SUBMODULE(test_chimera, m) { + Py_INCREF(&PyChimera_Type); + + m.def("make", []() -> Chimera { return Chimera{}; }); + + // it takes and it takes and it takes + m.def("take", [](Chimera c) { + c.x++; + return c.x; + }); + + m.def("take_ptr", [](Chimera* c) { + c->x++; + return c->x; + }); + m.def("take_ref", [](Chimera& c) { + c.x++; + return c.x; + }); + m.def("take_wrap", [](std::reference_wrapper c) { + c.get().x++; + return c.get().x; + }); + + m.def("take_const_ptr", [](const Chimera* c) { return 10 * c->x; }); + m.def("take_const_ref", [](const Chimera& c) { return 10 * c.x; }); + m.def("take_const_wrap", + [](std::reference_wrapper c) { return 10 * c.get().x; }); + + m.def("get", []() -> Chimera { return *shared; }); + + m.def("get_ptr", []() -> Chimera* { return shared; }); + m.def("get_ref", []() -> Chimera& { return *shared; }); + m.def("get_wrap", + []() -> std::reference_wrapper { return std::ref(*shared); }); + m.def("get_const_ptr", []() -> const Chimera* { + shared_const->x++; + return shared_const; + }); + m.def("get_const_ref", []() -> const Chimera& { + shared_const->x++; + return *shared_const; + }); + m.def("get_const_wrap", []() -> std::reference_wrapper { + shared_const->x++; + return std::cref(*shared_const); + }); + + m.def("roundtrip", [](Chimera c) -> Chimera { + c.x++; + return c; + }); + m.def("roundtrip_ptr", [](Chimera* c) -> Chimera* { + c->x++; + return c; + }); + m.def("roundtrip_ref", [](Chimera& c) -> Chimera& { + c.x++; + return c; + }); + m.def("roundtrip_wrap", + [](std::reference_wrapper c) -> std::reference_wrapper { + c.get().x++; + return c; + }); +} \ No newline at end of file diff --git a/tests/test_chimera.py b/tests/test_chimera.py new file mode 100644 index 0000000000..1394d02622 --- /dev/null +++ b/tests/test_chimera.py @@ -0,0 +1,69 @@ +# -*- coding: utf-8 -*- +import pytest + +import env # noqa: F401 + +from pybind11_tests import test_chimera as m + + + +def test_make(): + assert m.make().x == 1 + + +def test_take(): + c = m.make() + assert m.take(c) == 2 + assert m.take(c) == 2 + assert m.take_ptr(c) == 2 + assert m.take_ptr(c) == 3 + assert m.take_ref(c) == 4 + assert m.take_ref(c) == 5 + assert m.take_wrap(c) == 6 + assert m.take_wrap(c) == 7 + + assert m.take_const_ptr(c) == 70 + assert m.take_const_ref(c) == 70 + assert m.take_const_wrap(c) == 70 + + +def test_get(): + """The get calls all return a variant of the same shared pointer.""" + assert m.get().x == 1 + v = m.get_ptr() + assert v.x == 1 + v.x = 10 + assert m.get_ptr().x == 10 + v.x = 11 + assert m.get_ref().x == 11 + v.x = 12 + assert m.get_wrap().x == 12 + v.x = 13 + assert m.get().x == 13 + + +def test_get_const(): + v = m.get_const_ptr() + assert v.x == 2 + assert m.get_const_ref().x == 3 + assert m.get_const_wrap().x == 4 + assert m.get_const_ptr().x == 5 + assert m.get_const_ref().x == 6 + assert m.get_const_wrap().x == 7 + assert v.x == 7 + + with pytest.raises(ValueError) as excinfo: + v.x = 1 # immutable + + +def test_roundtrip(): + c = m.make() + assert c.x == 1 + assert m.roundtrip(c).x == 2 + assert m.roundtrip(c).x == 2 + + assert m.roundtrip_ptr(c).x == 2 + assert m.roundtrip_ref(c).x == 3 + assert m.roundtrip_wrap(c).x == 4 + + From 4cd06d35eea6474f52545d8e38e1c905f1fda230 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Tue, 1 Dec 2020 20:39:53 -0800 Subject: [PATCH 05/31] Add passing a const to non-const method. --- tests/test_chimera.cpp | 2 +- tests/test_chimera.py | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index 20a6e85915..6de126df78 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -45,7 +45,6 @@ PyObject* PyChimera_getattro(PyObject* self, PyObject* name) { if (std::string_view(attr) == "x") { return PyLong_FromLong(custom->value->x); } - return PyObject_GenericGetAttr(self, name); } @@ -225,6 +224,7 @@ TEST_SUBMODULE(test_chimera, m) { Py_INCREF(&PyChimera_Type); m.def("make", []() -> Chimera { return Chimera{}; }); + m.def("reset", [](int x) { shared->x = x; shared_const->x = x; }); // it takes and it takes and it takes m.def("take", [](Chimera c) { diff --git a/tests/test_chimera.py b/tests/test_chimera.py index 1394d02622..62086adfb2 100644 --- a/tests/test_chimera.py +++ b/tests/test_chimera.py @@ -29,6 +29,7 @@ def test_take(): def test_get(): """The get calls all return a variant of the same shared pointer.""" + m.reset(1) assert m.get().x == 1 v = m.get_ptr() assert v.x == 1 @@ -43,6 +44,7 @@ def test_get(): def test_get_const(): + m.reset(1) v = m.get_const_ptr() assert v.x == 2 assert m.get_const_ref().x == 3 @@ -67,3 +69,12 @@ def test_roundtrip(): assert m.roundtrip_wrap(c).x == 4 +def test_roundtrip_const(): + m.reset(1) + # by value, so the const ref is converted. + assert m.roundtrip(m.get_const_ref()).x == 3 + + # by reference, so it's not converted. + with pytest.raises(TypeError) as excinfo: + assert m.roundtrip_ref(m.get_const_ref()).x == 4 + From 5abd823d767710dccc9ce97e411daf318827c37f Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Tue, 1 Dec 2020 20:51:30 -0800 Subject: [PATCH 06/31] Demonstrate non-const conversion of reference_wrapper in tests. Apply formatting presubmit check. --- tests/test_builtin_casters.py | 10 +++++----- tests/test_chimera.cpp | 14 +++++++------- tests/test_chimera.py | 4 +--- 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/tests/test_builtin_casters.py b/tests/test_builtin_casters.py index 1338909595..1f9c2ebe9e 100644 --- a/tests/test_builtin_casters.py +++ b/tests/test_builtin_casters.py @@ -325,15 +325,15 @@ def test_reference_wrapper(): m.refwrap_usertype(None) assert "incompatible function arguments" in str(excinfo.value) - assert m.refwrap_lvalue_const().value == 1 - # const-ness is not propagated. - # m.refwrap_lvalue_const().value =2 - # assert m.refwrap_lvalue_const().value == 1 - assert m.refwrap_lvalue().value == 1 m.refwrap_lvalue().value = 4 assert m.refwrap_lvalue().value == 4 + # const-ness is not propagated. + assert m.refwrap_lvalue_const().value == 1 + m.refwrap_lvalue_const().value = 2 + assert m.refwrap_lvalue_const().value == 2 + a1 = m.refwrap_list(copy=True) a2 = m.refwrap_list(copy=True) assert [x.value for x in a1] == [2, 3] diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index 6de126df78..1fcc0556cb 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -256,13 +256,13 @@ TEST_SUBMODULE(test_chimera, m) { m.def("get_ref", []() -> Chimera& { return *shared; }); m.def("get_wrap", []() -> std::reference_wrapper { return std::ref(*shared); }); - m.def("get_const_ptr", []() -> const Chimera* { - shared_const->x++; - return shared_const; + m.def("get_const_ptr", []() -> const Chimera* { + shared_const->x++; + return shared_const; }); - m.def("get_const_ref", []() -> const Chimera& { - shared_const->x++; - return *shared_const; + m.def("get_const_ref", []() -> const Chimera& { + shared_const->x++; + return *shared_const; }); m.def("get_const_wrap", []() -> std::reference_wrapper { shared_const->x++; @@ -286,4 +286,4 @@ TEST_SUBMODULE(test_chimera, m) { c.get().x++; return c; }); -} \ No newline at end of file +} diff --git a/tests/test_chimera.py b/tests/test_chimera.py index 62086adfb2..ac285e294e 100644 --- a/tests/test_chimera.py +++ b/tests/test_chimera.py @@ -6,7 +6,6 @@ from pybind11_tests import test_chimera as m - def test_make(): assert m.make().x == 1 @@ -76,5 +75,4 @@ def test_roundtrip_const(): # by reference, so it's not converted. with pytest.raises(TypeError) as excinfo: - assert m.roundtrip_ref(m.get_const_ref()).x == 4 - + assert m.roundtrip_ref(m.get_const_ref()).x == 4 From c5bd1040ef183ccdf08c34bf913b0e401cfc39fc Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Tue, 1 Dec 2020 21:32:41 -0800 Subject: [PATCH 07/31] Fix build errors from presubmit checks. --- tests/test_chimera.cpp | 45 ++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index 1fcc0556cb..9d8e3fda68 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include "pybind11_tests.h" @@ -39,11 +40,14 @@ PyObject* PyChimera_getattro(PyObject* self, PyObject* name) { const char* attr = nullptr; if (PyBytes_Check(name)) { attr = PyBytes_AsString(name); - } else if (PyUnicode_Check(name)) { + } +#if PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 3 + if (PyUnicode_Check(name)) { attr = PyUnicode_AsUTF8(name); } - if (std::string_view(attr) == "x") { - return PyLong_FromLong(custom->value->x); +#endif + if (attr != nullptr && strcmp(attr, "x") == 0) { + return PyLong_FromLongLong(static_cast(custom->value->x)); } return PyObject_GenericGetAttr(self, name); } @@ -55,10 +59,13 @@ int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { const char* attr = nullptr; if (PyBytes_Check(name)) { attr = PyBytes_AsString(name); - } else if (PyUnicode_Check(name)) { + } +#if PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 3 + if (PyUnicode_Check(name)) { attr = PyUnicode_AsUTF8(name); } - if (std::string_view(attr) == "x") { +#endif + if (attr != nullptr && strcmp(attr, "x") == 0) { if (!PyLong_Check(value)) { PyErr_Format(PyExc_ValueError, "Cannot set a non-numeric value"); return -1; @@ -67,7 +74,7 @@ int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { PyErr_Format(PyExc_ValueError, "Instance is immutable; cannot set values"); return -1; } - custom->value->x = static_cast(PyLong_AsLong(value)); + custom->value->x = static_cast(PyLong_AsLongLong(value)); return 0; } @@ -76,17 +83,19 @@ int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { void PyChimera_dealloc(PyObject* self); -static PyTypeObject PyChimera_Type = { - .ob_base = PyVarObject_HEAD_INIT(nullptr, 0) /**/ - .tp_name = - "google3.experimental.users.lar.python.test_custom.Chimera", - .tp_basicsize = sizeof(PyChimera), - .tp_dealloc = &PyChimera_dealloc, - .tp_getattro = &PyChimera_getattro, - .tp_setattro = &PyChimera_setattro, - .tp_flags = Py_TPFLAGS_DEFAULT, - .tp_doc = "Chimera objects", -}; +static PyTypeObject PyChimera_Type = []{ + PyTypeObject tmp{ + PyVarObject_HEAD_INIT(nullptr, 0) /**/ + "pybind11_tests.test_chimera.Chimera", + sizeof(PyChimera), + }; + tmp.tp_dealloc = &PyChimera_dealloc; + tmp.tp_getattro = &PyChimera_getattro; + tmp.tp_setattro = &PyChimera_setattro; + tmp.tp_flags = Py_TPFLAGS_DEFAULT; + tmp.tp_doc = "Chimera objects"; + return tmp; +}(); static std::unordered_map* mapping = new std::unordered_map(); @@ -221,6 +230,8 @@ static Chimera* shared = new Chimera(); static Chimera* shared_const = new Chimera(); TEST_SUBMODULE(test_chimera, m) { + + Py_INCREF(&PyChimera_Type); m.def("make", []() -> Chimera { return Chimera{}; }); From f0e41f5aaf08edd075489af028abceef619595c1 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Tue, 1 Dec 2020 21:52:11 -0800 Subject: [PATCH 08/31] Try and fix a few more CI errors --- tests/test_chimera.cpp | 76 ++++++++++++++++++++++++++++++++++-------- tests/test_chimera.py | 4 +-- 2 files changed, 64 insertions(+), 16 deletions(-) diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index 9d8e3fda68..8bde3e76bc 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -7,6 +7,7 @@ BSD-style license that can be found in the LICENSE file. */ #include + #include #include @@ -41,7 +42,7 @@ PyObject* PyChimera_getattro(PyObject* self, PyObject* name) { if (PyBytes_Check(name)) { attr = PyBytes_AsString(name); } -#if PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 3 +#if PY_VERSION_HEX > 0x03030000 if (PyUnicode_Check(name)) { attr = PyUnicode_AsUTF8(name); } @@ -60,7 +61,7 @@ int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { if (PyBytes_Check(name)) { attr = PyBytes_AsString(name); } -#if PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 3 +#if PY_VERSION_HEX > 0x03030000 if (PyUnicode_Check(name)) { attr = PyUnicode_AsUTF8(name); } @@ -83,19 +84,66 @@ int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { void PyChimera_dealloc(PyObject* self); -static PyTypeObject PyChimera_Type = []{ - PyTypeObject tmp{ +static PyTypeObject PyChimera_Type{ PyVarObject_HEAD_INIT(nullptr, 0) /**/ - "pybind11_tests.test_chimera.Chimera", - sizeof(PyChimera), - }; - tmp.tp_dealloc = &PyChimera_dealloc; - tmp.tp_getattro = &PyChimera_getattro; - tmp.tp_setattro = &PyChimera_setattro; - tmp.tp_flags = Py_TPFLAGS_DEFAULT; - tmp.tp_doc = "Chimera objects"; - return tmp; -}(); + "pybind11_tests.test_chimera.Chimera", /* tp_name */ + sizeof(PyChimera), /* tp_basicsize */ + 0, /* tp_itemsize */ + &PyChimera_dealloc, /* tp_dealloc */ +#if PY_VERSION_HEX < 0x03080000 + nullptr, /* tp_print */ +#else + 0, /* tp_vectorcall_offset */ +#endif + 0, /* tp_getattr */ + 0, /* tp_setattr */ + 0, /* tp_as_async */ + 0, /* tp_repr */ + 0, /* tp_as_number */ + 0, /* tp_as_sequence */ + 0, /* tp_as_mapping */ + 0, /* tp_hash */ + 0, /* tp_call */ + 0, /* tp_str */ + &PyChimera_getattro, /* tp_getattro */ + &PyChimera_setattro, /* tp_setattro */ + 0, /* tp_as_buffer */ + Py_TPFLAGS_DEFAULT, /* tp_flags */ + "Chimera objects", /* tp_doc */ + 0, /* tp_traverse */ + 0, /* tp_clear */ + 0, /* tp_richcompare */ + 0, /* tp_weaklistoffset */ + 0, /* tp_iter */ + 0, /* tp_iternext */ + 0, /* tp_methods */ + 0, /* tp_members */ + 0, /* tp_getset */ + 0, /* tp_base */ + 0, /* tp_dict */ + 0, /* tp_descr_get */ + 0, /* tp_descr_set */ + 0, /* tp_dictoffset */ + 0, /* tp_init */ + 0, /* tp_alloc */ + 0, /* tp_new */ + 0, /* tp_free */ + 0, /* tp_is_gc */ + 0, /* tp_bases */ + 0, /* tp_mro */ + 0, /* tp_cache */ + 0, /* tp_subclasses */ + 0, /* tp_weaklist */ + 0, /* tp_del */ + 0, /* tp_version_tag */ +#if PY_VERSION_HEX > 0x03040000 + 0, /* tp_finalize */ +#if PY_VERSION_HEX > 0x03080000 + 0, /* tp_vectorcall */ +#endif +#endif +}; + static std::unordered_map* mapping = new std::unordered_map(); diff --git a/tests/test_chimera.py b/tests/test_chimera.py index ac285e294e..11b210fdb4 100644 --- a/tests/test_chimera.py +++ b/tests/test_chimera.py @@ -53,7 +53,7 @@ def test_get_const(): assert m.get_const_wrap().x == 7 assert v.x == 7 - with pytest.raises(ValueError) as excinfo: + with pytest.raises(ValueError): v.x = 1 # immutable @@ -74,5 +74,5 @@ def test_roundtrip_const(): assert m.roundtrip(m.get_const_ref()).x == 3 # by reference, so it's not converted. - with pytest.raises(TypeError) as excinfo: + with pytest.raises(TypeError): assert m.roundtrip_ref(m.get_const_ref()).x == 4 From b56a568d8a0884ccdf7f673493593fb67e59a369 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Tue, 1 Dec 2020 22:09:54 -0800 Subject: [PATCH 09/31] More CI fixes. --- tests/test_chimera.cpp | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index 8bde3e76bc..11b518949e 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -6,8 +6,6 @@ All rights reserved. Use of this source code is governed by a BSD-style license that can be found in the LICENSE file. */ -#include - #include #include @@ -67,7 +65,11 @@ int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { } #endif if (attr != nullptr && strcmp(attr, "x") == 0) { - if (!PyLong_Check(value)) { + if (!PyLong_Check(value) +#if PY_MAJOR_VERSION < 3 + || !PyInt_Check(value) +#endif +) { PyErr_Format(PyExc_ValueError, "Cannot set a non-numeric value"); return -1; } @@ -75,6 +77,12 @@ int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { PyErr_Format(PyExc_ValueError, "Instance is immutable; cannot set values"); return -1; } +#if PY_MAJOR_VERSION < 3 + if (PyInt_Check(value)) { + custom->value->x = static_cast(PyInt_AsLong(value)); + return 0; + } +#endif custom->value->x = static_cast(PyLong_AsLongLong(value)); return 0; } @@ -138,9 +146,12 @@ static PyTypeObject PyChimera_Type{ 0, /* tp_version_tag */ #if PY_VERSION_HEX > 0x03040000 0, /* tp_finalize */ +#endif #if PY_VERSION_HEX > 0x03080000 0, /* tp_vectorcall */ #endif +#if PY_VERSION_HEX > 0x03090000 + 0, /* tp_am_send */ #endif }; From d71cd7a7372c6436d76b4cbc6538bf759ae4f02f Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Tue, 1 Dec 2020 22:33:16 -0800 Subject: [PATCH 10/31] More CI fixups. --- tests/test_chimera.cpp | 45 ++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index 11b518949e..090cb4cb36 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -16,6 +16,15 @@ #include "pybind11_tests.h" +#if PY_VERSION_HEX > 0x03000000 +#define PyInt_Check PyLong_Check +#define PyInt_AsLong PyLong_AsLongLong +#endif + +#if !defined(CYTHON_COMPILING_IN_PYPY) +#define CYTHON_COMPILING_IN_PYPY 0 +#endif + /// C++ type class Chimera { public: @@ -65,25 +74,20 @@ int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { } #endif if (attr != nullptr && strcmp(attr, "x") == 0) { - if (!PyLong_Check(value) -#if PY_MAJOR_VERSION < 3 - || !PyInt_Check(value) -#endif -) { - PyErr_Format(PyExc_ValueError, "Cannot set a non-numeric value"); + if (!PyLong_Check(value) && !PyInt_Check(value)) { + // "Cannot set a non-numeric value of type %s" + PyErr_SetObject(PyExc_ValueError, value); return -1; } if (custom->is_immutable) { PyErr_Format(PyExc_ValueError, "Instance is immutable; cannot set values"); return -1; } -#if PY_MAJOR_VERSION < 3 - if (PyInt_Check(value)) { + if (PyLong_Check(value)) { + custom->value->x = static_cast(PyLong_AsLongLong(value)); + } else { custom->value->x = static_cast(PyInt_AsLong(value)); - return 0; - } -#endif - custom->value->x = static_cast(PyLong_AsLongLong(value)); + } return 0; } @@ -98,11 +102,7 @@ static PyTypeObject PyChimera_Type{ sizeof(PyChimera), /* tp_basicsize */ 0, /* tp_itemsize */ &PyChimera_dealloc, /* tp_dealloc */ -#if PY_VERSION_HEX < 0x03080000 - nullptr, /* tp_print */ -#else - 0, /* tp_vectorcall_offset */ -#endif + 0, /* tp_print | tp_vectorcall_offset */ 0, /* tp_getattr */ 0, /* tp_setattr */ 0, /* tp_as_async */ @@ -144,14 +144,17 @@ static PyTypeObject PyChimera_Type{ 0, /* tp_weaklist */ 0, /* tp_del */ 0, /* tp_version_tag */ -#if PY_VERSION_HEX > 0x03040000 +#if PY_VERSION_HEX >= 0x030400a1 0, /* tp_finalize */ #endif -#if PY_VERSION_HEX > 0x03080000 +#if PY_VERSION_HEX >= 0x030800b1 0, /* tp_vectorcall */ #endif -#if PY_VERSION_HEX > 0x03090000 - 0, /* tp_am_send */ +#if PY_VERSION_HEX >= 0x030800b4 && PY_VERSION_HEX < 0x03090000 + 0, /* tp_print */ +#endif +#if CYTHON_COMPILING_IN_PYPY && PYPY_VERSION_NUM+0 >= 0x06000000 + 0, /* tp_pypy_flags */ #endif }; From 3c696352caaffb2fd6456a58b99e29d53e599383 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Tue, 1 Dec 2020 22:40:20 -0800 Subject: [PATCH 11/31] Try and get PyPy to work. --- tests/test_chimera.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index 090cb4cb36..69872e2cef 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -21,10 +21,6 @@ #define PyInt_AsLong PyLong_AsLongLong #endif -#if !defined(CYTHON_COMPILING_IN_PYPY) -#define CYTHON_COMPILING_IN_PYPY 0 -#endif - /// C++ type class Chimera { public: @@ -153,7 +149,7 @@ static PyTypeObject PyChimera_Type{ #if PY_VERSION_HEX >= 0x030800b4 && PY_VERSION_HEX < 0x03090000 0, /* tp_print */ #endif -#if CYTHON_COMPILING_IN_PYPY && PYPY_VERSION_NUM+0 >= 0x06000000 +#if defined(PYPY_VERSION) && PYPY_VERSION_NUM+0 >= 0x06000000 0, /* tp_pypy_flags */ #endif }; From 8f32ba3b5270698cac1c48570d6035c68d70b8e9 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Tue, 1 Dec 2020 22:48:11 -0800 Subject: [PATCH 12/31] Additional minor fixups. Getting close to CI green. --- tests/test_chimera.cpp | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index 69872e2cef..a192ea6887 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -16,11 +16,6 @@ #include "pybind11_tests.h" -#if PY_VERSION_HEX > 0x03000000 -#define PyInt_Check PyLong_Check -#define PyInt_AsLong PyLong_AsLongLong -#endif - /// C++ type class Chimera { public: @@ -70,21 +65,24 @@ int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { } #endif if (attr != nullptr && strcmp(attr, "x") == 0) { - if (!PyLong_Check(value) && !PyInt_Check(value)) { - // "Cannot set a non-numeric value of type %s" - PyErr_SetObject(PyExc_ValueError, value); - return -1; - } if (custom->is_immutable) { PyErr_Format(PyExc_ValueError, "Instance is immutable; cannot set values"); return -1; } + if (PyLong_Check(value)) { custom->value->x = static_cast(PyLong_AsLongLong(value)); - } else { + return 0; + } +#if PY_VERSION_HEX < 0x03000000 + if (PyInt_Check(value)) { custom->value->x = static_cast(PyInt_AsLong(value)); + return 0; } - return 0; +#endif + // "Cannot set a non-numeric value of type %s" + PyErr_SetObject(PyExc_ValueError, value); + return -1; } return PyObject_GenericSetAttr(self, name, value); @@ -146,10 +144,10 @@ static PyTypeObject PyChimera_Type{ #if PY_VERSION_HEX >= 0x030800b1 0, /* tp_vectorcall */ #endif -#if PY_VERSION_HEX >= 0x030800b4 && PY_VERSION_HEX < 0x03090000 +#if PY_VERSION_HEX >= 0x030800b4 && PY_VERSION_HEX < 0x03080600 0, /* tp_print */ #endif -#if defined(PYPY_VERSION) && PYPY_VERSION_NUM+0 >= 0x06000000 +#if defined(PYPY_VERSION) 0, /* tp_pypy_flags */ #endif }; From d144e59297ac7bd46f69d81c65d3c4e8ffbc1a60 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Tue, 1 Dec 2020 23:10:49 -0800 Subject: [PATCH 13/31] More ci fixes? --- tests/test_chimera.cpp | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index a192ea6887..ada36ff9ea 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -24,7 +24,7 @@ class Chimera { /// Python wrapper for C++ type which supports mutable/immutable variants. typedef struct PyChimera { - PyObject_HEAD; + PyObject_HEAD Chimera* value; @@ -90,6 +90,14 @@ int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { void PyChimera_dealloc(PyObject* self); + +/* https://github.com/cython/cython/issues/3474 */ +#if defined(__GNUC__) || defined(__clang__) && PY_VERSION_HEX >= 0x030800b4 && PY_VERSION_HEX < 0x03090000 +#define PY38_WARNING_WORKAROUND_ENABLED 1 +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" +#endif + static PyTypeObject PyChimera_Type{ PyVarObject_HEAD_INIT(nullptr, 0) /**/ "pybind11_tests.test_chimera.Chimera", /* tp_name */ @@ -144,7 +152,7 @@ static PyTypeObject PyChimera_Type{ #if PY_VERSION_HEX >= 0x030800b1 0, /* tp_vectorcall */ #endif -#if PY_VERSION_HEX >= 0x030800b4 && PY_VERSION_HEX < 0x03080600 +#if PY_VERSION_HEX >= 0x030800b4 && PY_VERSION_HEX < 0x03090000 0, /* tp_print */ #endif #if defined(PYPY_VERSION) @@ -153,6 +161,11 @@ static PyTypeObject PyChimera_Type{ }; +#if defined(PY38_WARNING_WORKAROUND_ENABLED) +#undef PY38_WARNING_WORKAROUND_ENABLED +#pragma GCC diagnostic pop +#endif + static std::unordered_map* mapping = new std::unordered_map(); From a7ec72e935b565ca382f15d5f6ae2808be89c114 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Wed, 2 Dec 2020 09:57:49 -0800 Subject: [PATCH 14/31] fix clang-tidy warnings from presubmit --- tests/test_chimera.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index ada36ff9ea..642ca54ca1 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include "pybind11_tests.h" @@ -33,7 +34,7 @@ typedef struct PyChimera { } PyChimera; PyObject* PyChimera_getattro(PyObject* self, PyObject* name) { - PyChimera* custom = reinterpret_cast(self); + auto* custom = reinterpret_cast(self); assert(custom != nullptr); const char* attr = nullptr; @@ -52,7 +53,7 @@ PyObject* PyChimera_getattro(PyObject* self, PyObject* name) { } int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { - PyChimera* custom = reinterpret_cast(self); + auto* custom = reinterpret_cast(self); assert(custom != nullptr); const char* attr = nullptr; @@ -170,7 +171,7 @@ static std::unordered_map* mapping = new std::unordered_map(); void PyChimera_dealloc(PyObject* self) { - PyChimera* custom = reinterpret_cast(self); + auto* custom = reinterpret_cast(self); auto it = mapping->find(custom->value); if (it != mapping->end()) { mapping->erase(it); @@ -242,7 +243,10 @@ struct type_caster { // construct a mutable python type owning src. static handle cast(Chimera&& src, return_value_policy, handle) { - return PyChimera_new(new Chimera(std::move(src)), true, false); + std::unique_ptr ptr(new Chimera(std::move(src))); + handle h = PyChimera_new(ptr.get(), true, false); + if (h.ptr() != nullptr) ptr.release(); + return h; } // Convert Python->C++. From e1aca4b6a9f351abc47ff99ded528d7c5c2f2b0e Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Wed, 2 Dec 2020 10:37:30 -0800 Subject: [PATCH 15/31] fix more clang-tidy warnings --- tests/test_chimera.cpp | 58 +++++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index 642ca54ca1..cebe058ced 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -34,8 +34,8 @@ typedef struct PyChimera { } PyChimera; PyObject* PyChimera_getattro(PyObject* self, PyObject* name) { - auto* custom = reinterpret_cast(self); - assert(custom != nullptr); + auto* chimera = reinterpret_cast(self); + assert(chimera != nullptr); const char* attr = nullptr; if (PyBytes_Check(name)) { @@ -47,14 +47,14 @@ PyObject* PyChimera_getattro(PyObject* self, PyObject* name) { } #endif if (attr != nullptr && strcmp(attr, "x") == 0) { - return PyLong_FromLongLong(static_cast(custom->value->x)); + return PyLong_FromLongLong(static_cast(chimera->value->x)); } return PyObject_GenericGetAttr(self, name); } int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { - auto* custom = reinterpret_cast(self); - assert(custom != nullptr); + auto* chimera = reinterpret_cast(self); + assert(chimera != nullptr); const char* attr = nullptr; if (PyBytes_Check(name)) { @@ -66,18 +66,18 @@ int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { } #endif if (attr != nullptr && strcmp(attr, "x") == 0) { - if (custom->is_immutable) { + if (chimera->is_immutable) { PyErr_Format(PyExc_ValueError, "Instance is immutable; cannot set values"); return -1; } if (PyLong_Check(value)) { - custom->value->x = static_cast(PyLong_AsLongLong(value)); + chimera->value->x = static_cast(PyLong_AsLongLong(value)); return 0; } #if PY_VERSION_HEX < 0x03000000 if (PyInt_Check(value)) { - custom->value->x = static_cast(PyInt_AsLong(value)); + chimera->value->x = static_cast(PyInt_AsLong(value)); return 0; } #endif @@ -171,18 +171,18 @@ static std::unordered_map* mapping = new std::unordered_map(); void PyChimera_dealloc(PyObject* self) { - auto* custom = reinterpret_cast(self); - auto it = mapping->find(custom->value); + auto* chimera = reinterpret_cast(self); + auto it = mapping->find(chimera->value); if (it != mapping->end()) { mapping->erase(it); } - if (custom->is_owned) { - delete custom->value; + if (chimera->is_owned) { + delete chimera->value; } Py_TYPE(self)->tp_free(self); } -PyObject* PyChimera_new(Chimera* value, bool is_owned, bool is_immutable) { +PyObject* PyChimera_new(Chimera* value, bool is_immutable) { if (PyType_Ready(&PyChimera_Type) != 0) { return nullptr; } @@ -192,26 +192,38 @@ PyObject* PyChimera_new(Chimera* value, bool is_owned, bool is_immutable) { } mapping->emplace(value, self); self->value = value; - self->is_owned = is_owned; + self->is_owned = false; self->is_immutable = is_immutable; return reinterpret_cast(self); } +PyObject* PyChimera_new(std::unique_ptr value) { + PyObject* self = PyChimera_new(value.get(), false); + if (self != nullptr) { + auto* chimera = reinterpret_cast(self); + chimera->value = value.release(); + chimera->is_owned = true; + } + return self; +} + + PyObject* PyChimera_reference(Chimera* value, bool is_immutable) { auto it = mapping->find(value); if (it != mapping->end()) { - PyChimera* self = static_cast(it->second); + auto* chimera = static_cast(it->second); if (!is_immutable) { - self->is_immutable = false; + chimera->is_immutable = false; } - Py_INCREF(self); - return reinterpret_cast(self); + Py_INCREF(chimera); + return reinterpret_cast(chimera); } - return PyChimera_new(value, false, is_immutable); + return PyChimera_new(value, is_immutable); } -/// pybind11 typecaster which returns python wrapper type. - +/// pybind11 type_caster which returns custom PyChimera python wrapper instead +/// of a pybind11 generated type; this is used to make const* and const& immutable +/// in python. namespace pybind11 { namespace detail { @@ -244,9 +256,7 @@ struct type_caster { // construct a mutable python type owning src. static handle cast(Chimera&& src, return_value_policy, handle) { std::unique_ptr ptr(new Chimera(std::move(src))); - handle h = PyChimera_new(ptr.get(), true, false); - if (h.ptr() != nullptr) ptr.release(); - return h; + return PyChimera_new(std::move(ptr)); } // Convert Python->C++. From 005d3fea74c3ea595c5f090ca1d8566a571d0dbe Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Wed, 2 Dec 2020 10:50:54 -0800 Subject: [PATCH 16/31] minor comment and consistency cleanups --- tests/test_chimera.cpp | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index cebe058ced..a0ef740920 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -213,6 +213,8 @@ PyObject* PyChimera_reference(Chimera* value, bool is_immutable) { if (it != mapping->end()) { auto* chimera = static_cast(it->second); if (!is_immutable) { + // We have a single cache of C++ pointer to python object, so if any of the + // objects becomes immutable, they all become immutable. chimera->is_immutable = false; } Py_INCREF(chimera); @@ -253,19 +255,25 @@ struct type_caster { return cast(&src, policy, parent); } - // construct a mutable python type owning src. + // ... construct a mutable python type owning src. static handle cast(Chimera&& src, return_value_policy, handle) { std::unique_ptr ptr(new Chimera(std::move(src))); return PyChimera_new(std::move(ptr)); } + ~type_caster() { + if (chimera) PyDECREF(chimera); + } // Convert Python->C++. + // ... Merely capture the PyChimera pointer and do additional work in the + // conversion operator. bool load(handle src, bool) { - // ... either reference a wrapped c++ type or construct a new one. if (!PyObject_TypeCheck(src.ptr(), &PyChimera_Type)) { return false; } - custom = reinterpret_cast(src.ptr()); + assert(chimera == nullptr); + chimera = reinterpret_cast(src.ptr()); + Py_INCREF(chimera); return true; } @@ -281,40 +289,42 @@ struct type_caster { /*default is T&&*/ T_>>>>; // PYBIND11_TYPE_CASTER - operator const Chimera*() { return custom->value; } + operator const Chimera*() { return chimera->value; } operator const Chimera&() { - if (!custom || !custom->value) throw reference_cast_error(); - return *custom->value; + if (!chimera || !chimera->value) throw reference_cast_error(); + return *chimera->value; } operator Chimera*() { - if (custom->is_immutable) throw reference_cast_error(); - return custom->value; + if (chimera->is_immutable) throw reference_cast_error(); + return chimera->value; } operator Chimera&() { - if (!custom || !custom->value) throw reference_cast_error(); - if (custom->is_immutable) throw reference_cast_error(); - return *custom->value; + if (!chimera || !chimera->value) throw reference_cast_error(); + if (chimera->is_immutable) throw reference_cast_error(); + return *chimera->value; } operator Chimera&&() && { - if (!custom || !custom->value) throw reference_cast_error(); - owned = *custom->value; + if (!chimera || !chimera->value) throw reference_cast_error(); + owned = *chimera->value; return std::move(owned); } protected: - const PyChimera* custom; + const PyChimera* chimera = nullptr; Chimera owned; }; } // namespace detail } // namespace pybind11 +/// C++ module using pybind11 type_caster returning mutable/immutable +/// objects. + static Chimera* shared = new Chimera(); static Chimera* shared_const = new Chimera(); TEST_SUBMODULE(test_chimera, m) { - Py_INCREF(&PyChimera_Type); m.def("make", []() -> Chimera { return Chimera{}; }); From 4ac23ca2b31460eb791fc32fcdb05ebc82dc2ba8 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Wed, 2 Dec 2020 10:55:33 -0800 Subject: [PATCH 17/31] PyDECREF -> Py_DECREF --- tests/test_chimera.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index a0ef740920..488fd3e3b6 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -262,7 +262,7 @@ struct type_caster { } ~type_caster() { - if (chimera) PyDECREF(chimera); + if (chimera) Py_DECREF(chimera); } // Convert Python->C++. // ... Merely capture the PyChimera pointer and do additional work in the @@ -310,7 +310,7 @@ struct type_caster { } protected: - const PyChimera* chimera = nullptr; + PyChimera* chimera = nullptr; Chimera owned; }; From a3d9b18db172f9a36df577578068e0414b7c38ba Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Wed, 2 Dec 2020 11:20:10 -0800 Subject: [PATCH 18/31] copy/move constructors --- tests/test_chimera.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/test_chimera.cpp b/tests/test_chimera.cpp index 488fd3e3b6..c070ed1d73 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_chimera.cpp @@ -261,9 +261,24 @@ struct type_caster { return PyChimera_new(std::move(ptr)); } + type_caster() = default; + type_caster(type_caster&&) = default; + type_caster(const type_caster& other) : chimera(other.chimera) { + if (chimera) Py_INCREF(chimera); + } + + type_caster& operator=(type_caster&&) = default; + type_caster& operator=(const type_caster& other) { + if (chimera) Py_DECREF(chimera); + chimera = other.chimera; + if (chimera) Py_INCREF(chimera); + return *this; + } + ~type_caster() { if (chimera) Py_DECREF(chimera); } + // Convert Python->C++. // ... Merely capture the PyChimera pointer and do additional work in the // conversion operator. From 5b4fb9869c1316a85f97cc0f02f8a6653adfe49a Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Thu, 3 Dec 2020 10:59:16 -0800 Subject: [PATCH 19/31] Resolve codereview comments --- include/pybind11/cast.h | 2 +- tests/CMakeLists.txt | 2 +- tests/test_builtin_casters.py | 6 - ...era.cpp => test_freezable_type_caster.cpp} | 255 +++++++++--------- ...imera.py => test_freezable_type_caster.py} | 2 +- 5 files changed, 133 insertions(+), 134 deletions(-) rename tests/{test_chimera.cpp => test_freezable_type_caster.cpp} (51%) rename tests/{test_chimera.py => test_freezable_type_caster.py} (96%) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 39f065ce92..2052235aaa 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -968,7 +968,7 @@ template class type_caster> { subcaster_cast_op_type>::value || std::is_same::value), "std::reference_wrapper caster requires T to have a caster " - "with an `T &` operator or `const T&` operator"); + "with an operator `T &` or `const T&`"); public: bool load(handle src, bool convert) { return subcaster.load(src, convert); } static constexpr auto name = caster_t::name; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 04cce5c45b..611aae60fc 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -96,13 +96,13 @@ set(PYBIND11_TEST_FILES test_constants_and_functions.cpp test_copy_move.cpp test_custom_type_casters.cpp - test_chimera.cpp test_docstring_options.cpp test_eigen.cpp test_enum.cpp test_eval.cpp test_exceptions.cpp test_factory_constructors.cpp + test_freezable_type_caster.cpp test_gil_scoped.cpp test_iostream.cpp test_kwargs_and_defaults.cpp diff --git a/tests/test_builtin_casters.py b/tests/test_builtin_casters.py index 1f9c2ebe9e..99cdae29b6 100644 --- a/tests/test_builtin_casters.py +++ b/tests/test_builtin_casters.py @@ -326,13 +326,7 @@ def test_reference_wrapper(): assert "incompatible function arguments" in str(excinfo.value) assert m.refwrap_lvalue().value == 1 - m.refwrap_lvalue().value = 4 - assert m.refwrap_lvalue().value == 4 - - # const-ness is not propagated. assert m.refwrap_lvalue_const().value == 1 - m.refwrap_lvalue_const().value = 2 - assert m.refwrap_lvalue_const().value == 2 a1 = m.refwrap_list(copy=True) a2 = m.refwrap_list(copy=True) diff --git a/tests/test_chimera.cpp b/tests/test_freezable_type_caster.cpp similarity index 51% rename from tests/test_chimera.cpp rename to tests/test_freezable_type_caster.cpp index c070ed1d73..31ad4af004 100644 --- a/tests/test_chimera.cpp +++ b/tests/test_freezable_type_caster.cpp @@ -1,7 +1,12 @@ /* - tests/test_chimera.cpp -- This demonstrates a hybrid usage of pybind11, where the - type caster returns a hand-rolled python object type rather than relying on the - natural python bindings. + tests/test_freezable_type_caster.cpp + + This test verifies that const-ness is propagated through the type_caster framework. + The returned FreezableInt type is a custom type that is frozen when returned as a + const T or std::reference, and not frozen otherwise. + This test is somewhat complicated because it introduces a custom python type to + manage the frozen aspect of the type rather then relying on native pybind11 which + does not support such a feature. All rights reserved. Use of this source code is governed by a BSD-style license that can be found in the LICENSE file. @@ -18,24 +23,24 @@ #include "pybind11_tests.h" /// C++ type -class Chimera { +class FreezableInt { public: int64_t x = 1; }; /// Python wrapper for C++ type which supports mutable/immutable variants. -typedef struct PyChimera { +typedef struct PyFreezableInt { PyObject_HEAD - Chimera* value; + FreezableInt* value; bool is_immutable; bool is_owned; -} PyChimera; +} PyFreezableInt; -PyObject* PyChimera_getattro(PyObject* self, PyObject* name) { - auto* chimera = reinterpret_cast(self); - assert(chimera != nullptr); +PyObject* PyFreezableInt_getattro(PyObject* self, PyObject* name) { + auto* freezable = reinterpret_cast(self); + assert(freezable != nullptr); const char* attr = nullptr; if (PyBytes_Check(name)) { @@ -47,14 +52,14 @@ PyObject* PyChimera_getattro(PyObject* self, PyObject* name) { } #endif if (attr != nullptr && strcmp(attr, "x") == 0) { - return PyLong_FromLongLong(static_cast(chimera->value->x)); + return PyLong_FromLongLong(static_cast(freezable->value->x)); } return PyObject_GenericGetAttr(self, name); } -int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { - auto* chimera = reinterpret_cast(self); - assert(chimera != nullptr); +int PyFreezableInt_setattro(PyObject* self, PyObject* name, PyObject* value) { + auto* freezable = reinterpret_cast(self); + assert(freezable != nullptr); const char* attr = nullptr; if (PyBytes_Check(name)) { @@ -66,18 +71,18 @@ int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { } #endif if (attr != nullptr && strcmp(attr, "x") == 0) { - if (chimera->is_immutable) { + if (freezable->is_immutable) { PyErr_Format(PyExc_ValueError, "Instance is immutable; cannot set values"); return -1; } if (PyLong_Check(value)) { - chimera->value->x = static_cast(PyLong_AsLongLong(value)); + freezable->value->x = static_cast(PyLong_AsLongLong(value)); return 0; } #if PY_VERSION_HEX < 0x03000000 if (PyInt_Check(value)) { - chimera->value->x = static_cast(PyInt_AsLong(value)); + freezable->value->x = static_cast(PyInt_AsLong(value)); return 0; } #endif @@ -89,7 +94,7 @@ int PyChimera_setattro(PyObject* self, PyObject* name, PyObject* value) { return PyObject_GenericSetAttr(self, name, value); } -void PyChimera_dealloc(PyObject* self); +void PyFreezableInt_dealloc(PyObject* self); /* https://github.com/cython/cython/issues/3474 */ @@ -99,12 +104,12 @@ void PyChimera_dealloc(PyObject* self); #pragma GCC diagnostic ignored "-Wdeprecated-declarations" #endif -static PyTypeObject PyChimera_Type{ +static PyTypeObject PyFreezableInt_Type{ PyVarObject_HEAD_INIT(nullptr, 0) /**/ - "pybind11_tests.test_chimera.Chimera", /* tp_name */ - sizeof(PyChimera), /* tp_basicsize */ + "pybind11_tests.test_freezable.FreezableInt", /* tp_name */ + sizeof(PyFreezableInt), /* tp_basicsize */ 0, /* tp_itemsize */ - &PyChimera_dealloc, /* tp_dealloc */ + &PyFreezableInt_dealloc, /* tp_dealloc */ 0, /* tp_print | tp_vectorcall_offset */ 0, /* tp_getattr */ 0, /* tp_setattr */ @@ -116,11 +121,11 @@ static PyTypeObject PyChimera_Type{ 0, /* tp_hash */ 0, /* tp_call */ 0, /* tp_str */ - &PyChimera_getattro, /* tp_getattro */ - &PyChimera_setattro, /* tp_setattro */ + &PyFreezableInt_getattro, /* tp_getattro */ + &PyFreezableInt_setattro, /* tp_setattro */ 0, /* tp_as_buffer */ Py_TPFLAGS_DEFAULT, /* tp_flags */ - "Chimera objects", /* tp_doc */ + "FreezableInt objects", /* tp_doc */ 0, /* tp_traverse */ 0, /* tp_clear */ 0, /* tp_richcompare */ @@ -167,128 +172,127 @@ static PyTypeObject PyChimera_Type{ #pragma GCC diagnostic pop #endif -static std::unordered_map* mapping = - new std::unordered_map(); +static std::unordered_map mapping; -void PyChimera_dealloc(PyObject* self) { - auto* chimera = reinterpret_cast(self); - auto it = mapping->find(chimera->value); - if (it != mapping->end()) { - mapping->erase(it); +void PyFreezableInt_dealloc(PyObject* self) { + auto* freezable = reinterpret_cast(self); + auto it = mapping.find(freezable->value); + if (it != mapping.end()) { + mapping.erase(it); } - if (chimera->is_owned) { - delete chimera->value; + if (freezable->is_owned) { + delete freezable->value; } Py_TYPE(self)->tp_free(self); } -PyObject* PyChimera_new(Chimera* value, bool is_immutable) { - if (PyType_Ready(&PyChimera_Type) != 0) { +PyObject* PyFreezableInt_new(FreezableInt* value, bool is_immutable) { + if (PyType_Ready(&PyFreezableInt_Type) != 0) { return nullptr; } - PyChimera* self = PyObject_New(PyChimera, &PyChimera_Type); + PyFreezableInt* self = PyObject_New(PyFreezableInt, &PyFreezableInt_Type); if (!self) { return nullptr; } - mapping->emplace(value, self); + mapping.emplace(value, self); self->value = value; self->is_owned = false; self->is_immutable = is_immutable; return reinterpret_cast(self); } -PyObject* PyChimera_new(std::unique_ptr value) { - PyObject* self = PyChimera_new(value.get(), false); +PyObject* PyFreezableInt_new(std::unique_ptr value) { + PyObject* self = PyFreezableInt_new(value.get(), false); if (self != nullptr) { - auto* chimera = reinterpret_cast(self); - chimera->value = value.release(); - chimera->is_owned = true; + auto* freezable = reinterpret_cast(self); + freezable->value = value.release(); + freezable->is_owned = true; } return self; } -PyObject* PyChimera_reference(Chimera* value, bool is_immutable) { - auto it = mapping->find(value); - if (it != mapping->end()) { - auto* chimera = static_cast(it->second); +PyObject* PyFreezableInt_reference(FreezableInt* value, bool is_immutable) { + auto it = mapping.find(value); + if (it != mapping.end()) { + auto* freezable = static_cast(it->second); if (!is_immutable) { // We have a single cache of C++ pointer to python object, so if any of the // objects becomes immutable, they all become immutable. - chimera->is_immutable = false; + freezable->is_immutable = false; } - Py_INCREF(chimera); - return reinterpret_cast(chimera); + Py_INCREF(freezable); + return reinterpret_cast(freezable); } - return PyChimera_new(value, is_immutable); + return PyFreezableInt_new(value, is_immutable); } -/// pybind11 type_caster which returns custom PyChimera python wrapper instead +/// pybind11 type_caster which returns custom PyFreezableInt python wrapper instead /// of a pybind11 generated type; this is used to make const* and const& immutable /// in python. namespace pybind11 { namespace detail { template <> -struct type_caster { - static constexpr auto name = _(); +struct type_caster { + static constexpr auto name = _(); // C++ -> Python // ... construct an immutable python type referencing src. This isn't really // available in pybind11 and would have to be built by hand. - static handle cast(const Chimera* src, return_value_policy, handle) { + static handle cast(const FreezableInt* src, return_value_policy, handle) { if (!src) return pybind11::none().release(); - return PyChimera_reference(const_cast(src), true); + return PyFreezableInt_reference(const_cast(src), true); } - static handle cast(const Chimera& src, return_value_policy policy, + static handle cast(const FreezableInt& src, return_value_policy policy, handle parent) { return cast(&src, policy, parent); } // ... construct a mutable python type referencing src. This is the default // pybind11 path. - static handle cast(Chimera* src, return_value_policy, handle) { + static handle cast(FreezableInt* src, return_value_policy, handle) { if (!src) return pybind11::none().release(); - return PyChimera_reference(src, false); + return PyFreezableInt_reference(src, false); } - static handle cast(Chimera& src, return_value_policy policy, handle parent) { + static handle cast(FreezableInt& src, return_value_policy policy, handle parent) { return cast(&src, policy, parent); } // ... construct a mutable python type owning src. - static handle cast(Chimera&& src, return_value_policy, handle) { - std::unique_ptr ptr(new Chimera(std::move(src))); - return PyChimera_new(std::move(ptr)); + static handle cast(FreezableInt&& src, return_value_policy, handle) { + std::unique_ptr ptr(new FreezableInt(std::move(src))); + return PyFreezableInt_new(std::move(ptr)); } type_caster() = default; type_caster(type_caster&&) = default; - type_caster(const type_caster& other) : chimera(other.chimera) { - if (chimera) Py_INCREF(chimera); + type_caster(const type_caster& other) : freezable(other.freezable) { + if (freezable) Py_INCREF(freezable); } type_caster& operator=(type_caster&&) = default; type_caster& operator=(const type_caster& other) { - if (chimera) Py_DECREF(chimera); - chimera = other.chimera; - if (chimera) Py_INCREF(chimera); + if (freezable) Py_DECREF(freezable); + freezable = other.freezable; + if (freezable) Py_INCREF(freezable); return *this; } ~type_caster() { - if (chimera) Py_DECREF(chimera); + if (freezable) Py_DECREF(freezable); } // Convert Python->C++. - // ... Merely capture the PyChimera pointer and do additional work in the + // ... Merely capture the PyFreezableInt pointer and do additional work in the // conversion operator. bool load(handle src, bool) { - if (!PyObject_TypeCheck(src.ptr(), &PyChimera_Type)) { + if (!PyObject_TypeCheck(src.ptr(), &PyFreezableInt_Type)) { return false; } - assert(chimera == nullptr); - chimera = reinterpret_cast(src.ptr()); - Py_INCREF(chimera); + assert(freezable == nullptr); + freezable = reinterpret_cast(src.ptr()); + Py_INCREF(freezable); return true; } @@ -296,112 +300,113 @@ struct type_caster { // input parameter type. In this case we want to propagate const, etc. template using cast_op_type = conditional_t< - std::is_same, const Chimera*>::value, const Chimera*, + std::is_same, const FreezableInt*>::value, const FreezableInt*, conditional_t< - std::is_same, Chimera*>::value, Chimera*, - conditional_t::value, const Chimera&, - conditional_t::value, Chimera&, + std::is_same, FreezableInt*>::value, FreezableInt*, + conditional_t::value, const FreezableInt&, + conditional_t::value, FreezableInt&, /*default is T&&*/ T_>>>>; // PYBIND11_TYPE_CASTER - operator const Chimera*() { return chimera->value; } - operator const Chimera&() { - if (!chimera || !chimera->value) throw reference_cast_error(); - return *chimera->value; + operator const FreezableInt*() { return freezable->value; } + operator const FreezableInt&() { + if (!freezable || !freezable->value) throw reference_cast_error(); + return *freezable->value; } - operator Chimera*() { - if (chimera->is_immutable) throw reference_cast_error(); - return chimera->value; + operator FreezableInt*() { + if (freezable->is_immutable) throw reference_cast_error(); + return freezable->value; } - operator Chimera&() { - if (!chimera || !chimera->value) throw reference_cast_error(); - if (chimera->is_immutable) throw reference_cast_error(); - return *chimera->value; + operator FreezableInt&() { + if (!freezable || !freezable->value) throw reference_cast_error(); + if (freezable->is_immutable) throw reference_cast_error(); + return *freezable->value; } - operator Chimera&&() && { - if (!chimera || !chimera->value) throw reference_cast_error(); - owned = *chimera->value; + operator FreezableInt&&() && { + if (!freezable || !freezable->value) throw reference_cast_error(); + owned = *freezable->value; return std::move(owned); } protected: - PyChimera* chimera = nullptr; - Chimera owned; + PyFreezableInt* freezable = nullptr; + FreezableInt owned; }; } // namespace detail } // namespace pybind11 -/// C++ module using pybind11 type_caster returning mutable/immutable +/// C++ module using pybind11 type_caster returning mutable/immutable /// objects. -static Chimera* shared = new Chimera(); -static Chimera* shared_const = new Chimera(); +static FreezableInt shared; // returned by non-const get. +static FreezableInt shared_const; // returned by const get. -TEST_SUBMODULE(test_chimera, m) { +TEST_SUBMODULE(test_freezable_type_caster, m) { - Py_INCREF(&PyChimera_Type); + Py_INCREF(&PyFreezableInt_Type); - m.def("make", []() -> Chimera { return Chimera{}; }); - m.def("reset", [](int x) { shared->x = x; shared_const->x = x; }); + m.def("make", []() -> FreezableInt { return FreezableInt{}; }); + m.def("reset", [](int x) { shared.x = x; shared_const.x = x; }); // it takes and it takes and it takes - m.def("take", [](Chimera c) { + m.def("take", [](FreezableInt c) { c.x++; return c.x; }); - m.def("take_ptr", [](Chimera* c) { + m.def("take_ptr", [](FreezableInt* c) { c->x++; return c->x; }); - m.def("take_ref", [](Chimera& c) { + m.def("take_ref", [](FreezableInt& c) { c.x++; return c.x; }); - m.def("take_wrap", [](std::reference_wrapper c) { + m.def("take_wrap", [](std::reference_wrapper c) { c.get().x++; return c.get().x; }); - m.def("take_const_ptr", [](const Chimera* c) { return 10 * c->x; }); - m.def("take_const_ref", [](const Chimera& c) { return 10 * c.x; }); + m.def("take_const_ptr", [](const FreezableInt* c) { return 10 * c->x; }); + m.def("take_const_ref", [](const FreezableInt& c) { return 10 * c.x; }); m.def("take_const_wrap", - [](std::reference_wrapper c) { return 10 * c.get().x; }); + [](std::reference_wrapper c) { return 10 * c.get().x; }); - m.def("get", []() -> Chimera { return *shared; }); + m.def("get", []() -> FreezableInt { return shared; }); - m.def("get_ptr", []() -> Chimera* { return shared; }); - m.def("get_ref", []() -> Chimera& { return *shared; }); + m.def("get_ptr", []() -> FreezableInt* { return &shared; }); + m.def("get_ref", []() -> FreezableInt& { return shared; }); m.def("get_wrap", - []() -> std::reference_wrapper { return std::ref(*shared); }); - m.def("get_const_ptr", []() -> const Chimera* { - shared_const->x++; - return shared_const; + []() -> std::reference_wrapper { return std::ref(shared); }); + + m.def("get_const_ptr", []() -> const FreezableInt* { + shared_const.x++; + return &shared_const; }); - m.def("get_const_ref", []() -> const Chimera& { - shared_const->x++; - return *shared_const; + m.def("get_const_ref", []() -> const FreezableInt& { + shared_const.x++; + return shared_const; }); - m.def("get_const_wrap", []() -> std::reference_wrapper { - shared_const->x++; - return std::cref(*shared_const); + m.def("get_const_wrap", []() -> std::reference_wrapper { + shared_const.x++; + return std::cref(shared_const); }); - m.def("roundtrip", [](Chimera c) -> Chimera { + m.def("roundtrip", [](FreezableInt c) -> FreezableInt { c.x++; return c; }); - m.def("roundtrip_ptr", [](Chimera* c) -> Chimera* { + m.def("roundtrip_ptr", [](FreezableInt* c) -> FreezableInt* { c->x++; return c; }); - m.def("roundtrip_ref", [](Chimera& c) -> Chimera& { + m.def("roundtrip_ref", [](FreezableInt& c) -> FreezableInt& { c.x++; return c; }); m.def("roundtrip_wrap", - [](std::reference_wrapper c) -> std::reference_wrapper { + [](std::reference_wrapper c) -> std::reference_wrapper { c.get().x++; return c; }); diff --git a/tests/test_chimera.py b/tests/test_freezable_type_caster.py similarity index 96% rename from tests/test_chimera.py rename to tests/test_freezable_type_caster.py index 11b210fdb4..a6332126dd 100644 --- a/tests/test_chimera.py +++ b/tests/test_freezable_type_caster.py @@ -3,7 +3,7 @@ import env # noqa: F401 -from pybind11_tests import test_chimera as m +from pybind11_tests import test_freezable_type_caster as m def test_make(): From 8dcbc42dea73b2c47875db9914ad5bd4e18a2666 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Thu, 3 Dec 2020 14:30:27 -0800 Subject: [PATCH 20/31] more review comment fixes --- tests/test_freezable_type_caster.cpp | 5 +++++ tests/test_freezable_type_caster.py | 19 +++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/tests/test_freezable_type_caster.cpp b/tests/test_freezable_type_caster.cpp index 31ad4af004..2902dc3c80 100644 --- a/tests/test_freezable_type_caster.cpp +++ b/tests/test_freezable_type_caster.cpp @@ -410,4 +410,9 @@ TEST_SUBMODULE(test_freezable_type_caster, m) { c.get().x++; return c; }); + + m.def("roundtrip_const_ref", [](const FreezableInt& c) -> std::reference_wrapper { + return std::cref(c); + }); + } diff --git a/tests/test_freezable_type_caster.py b/tests/test_freezable_type_caster.py index a6332126dd..05b2c71373 100644 --- a/tests/test_freezable_type_caster.py +++ b/tests/test_freezable_type_caster.py @@ -70,9 +70,20 @@ def test_roundtrip(): def test_roundtrip_const(): m.reset(1) - # by value, so the const ref is converted. - assert m.roundtrip(m.get_const_ref()).x == 3 - # by reference, so it's not converted. + # NOTE: each call to get_const_ref increments x. + # by reference, the const ref is not converted. + with pytest.raises(TypeError): + m.roundtrip_ref(m.get_const_ref()) + + with pytest.raises(TypeError): + m.roundtrip_ptr(m.get_const_ref()) + with pytest.raises(TypeError): - assert m.roundtrip_ref(m.get_const_ref()).x == 4 + m.roundtrip_wrap(m.get_const_ref()) + + # by value, so the const ref is converted. + assert m.roundtrip(m.get_const_ref()).x == 6 # x + 1 + + # by const ref, so the conversion is ok. + assert m.roundtrip_const_ref(m.get_const_ref()).x == 6 From 9f77862411fa9c23b1d218b3bc20c35e8513a281 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Mon, 7 Dec 2020 10:23:15 -0800 Subject: [PATCH 21/31] review comments: remove spurious & --- include/pybind11/cast.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 2052235aaa..23a7a43df3 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -979,7 +979,7 @@ template class type_caster> { return caster_t::cast(&src.get(), policy, parent); } template using cast_op_type = std::reference_wrapper; - operator std::reference_wrapper() { return subcaster.operator subcaster_cast_op_type&(); } + operator std::reference_wrapper() { return subcaster.operator subcaster_cast_op_type(); } }; #define PYBIND11_TYPE_CASTER(type, py_name) \ From 7672e92e9c56a0d34809c38ded1ff376ab404e85 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Mon, 7 Dec 2020 20:06:14 -0800 Subject: [PATCH 22/31] Make the test fail even when the static_assert is commented out. This expands the test_freezable_type_caster a bit by: 1/ adding accessors .is_immutable and .addr to compare identity from python. 2/ Changing the default cast_op of the type_caster<> specialization to return a non-const value. In normal codepaths this is a reasonable default. 3/ adding roundtrip variants to exercise the by reference, by pointer and by reference_wrapper in all call paths. In conjunction with 2/, this demonstrates the failure case of the existing std::reference_wrpper conversion, which now loses const in a similar way that happens when using the default cast_op_type<>. --- tests/test_freezable_type_caster.cpp | 22 ++++++++++++++++---- tests/test_freezable_type_caster.py | 30 +++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/tests/test_freezable_type_caster.cpp b/tests/test_freezable_type_caster.cpp index 2902dc3c80..6f9678a475 100644 --- a/tests/test_freezable_type_caster.cpp +++ b/tests/test_freezable_type_caster.cpp @@ -54,6 +54,12 @@ PyObject* PyFreezableInt_getattro(PyObject* self, PyObject* name) { if (attr != nullptr && strcmp(attr, "x") == 0) { return PyLong_FromLongLong(static_cast(freezable->value->x)); } + if (attr != nullptr && strcmp(attr, "is_immutable") == 0) { + return PyBool_FromLong((freezable->is_immutable) ? 1 : 0); + } + if (attr != nullptr && strcmp(attr, "addr") == 0) { + return PyLong_FromLongLong(reinterpret_cast(freezable->value)); + } return PyObject_GenericGetAttr(self, name); } @@ -305,7 +311,8 @@ struct type_caster { std::is_same, FreezableInt*>::value, FreezableInt*, conditional_t::value, const FreezableInt&, conditional_t::value, FreezableInt&, - /*default is T&&*/ T_>>>>; + conditional_t::value, FreezableInt&&, + /* by-value is a reasonable default */ FreezableInt>>>>>; // PYBIND11_TYPE_CASTER operator const FreezableInt*() { return freezable->value; } @@ -327,6 +334,11 @@ struct type_caster { owned = *freezable->value; return std::move(owned); } + operator FreezableInt() { + if (!freezable || !freezable->value) throw reference_cast_error(); + FreezableInt v = *freezable->value; + return v; + } protected: PyFreezableInt* freezable = nullptr; @@ -410,9 +422,11 @@ TEST_SUBMODULE(test_freezable_type_caster, m) { c.get().x++; return c; }); - - m.def("roundtrip_const_ref", [](const FreezableInt& c) -> std::reference_wrapper { - return std::cref(c); + m.def("roundtrip_const_ref", [](const FreezableInt& c) -> const FreezableInt& { + return c; + }); + m.def("roundtrip_const_wrap", [](std::reference_wrapper c) -> std::reference_wrapper { + return c; }); } diff --git a/tests/test_freezable_type_caster.py b/tests/test_freezable_type_caster.py index 05b2c71373..8b535afd2c 100644 --- a/tests/test_freezable_type_caster.py +++ b/tests/test_freezable_type_caster.py @@ -32,6 +32,7 @@ def test_get(): assert m.get().x == 1 v = m.get_ptr() assert v.x == 1 + assert not v.is_immutable v.x = 10 assert m.get_ptr().x == 10 v.x = 11 @@ -46,6 +47,7 @@ def test_get_const(): m.reset(1) v = m.get_const_ptr() assert v.x == 2 + assert v.is_immutable assert m.get_const_ref().x == 3 assert m.get_const_wrap().x == 4 assert m.get_const_ptr().x == 5 @@ -53,8 +55,23 @@ def test_get_const(): assert m.get_const_wrap().x == 7 assert v.x == 7 + assert m.get_const_ref().is_immutable + assert m.get_const_wrap().is_immutable + assert m.get_const_ptr().is_immutable + + assert m.get_const_wrap().addr == m.get_const_ref().addr + assert m.get_const_wrap().addr == m.get_const_ptr().addr + assert m.get_const_ref().addr == m.get_const_ptr().addr + + # immutable + with pytest.raises(ValueError): + v.x = 1 + with pytest.raises(ValueError): + m.get_const_ref().x = 1 with pytest.raises(ValueError): - v.x = 1 # immutable + m.get_const_wrap().x = 1 + with pytest.raises(ValueError): + m.get_const_ptr().x = 1 def test_roundtrip(): @@ -67,6 +84,12 @@ def test_roundtrip(): assert m.roundtrip_ref(c).x == 3 assert m.roundtrip_wrap(c).x == 4 + assert m.roundtrip_ptr(m.get_ref()).x == 2 + assert m.roundtrip_ref(m.get_wrap()).x == 3 + assert m.roundtrip_wrap(m.get_ptr()).x == 4 + + assert m.roundtrip_const_ref(c).addr == c.addr + assert m.roundtrip_const_wrap(c).addr == c.addr def test_roundtrip_const(): m.reset(1) @@ -82,8 +105,13 @@ def test_roundtrip_const(): with pytest.raises(TypeError): m.roundtrip_wrap(m.get_const_ref()) + # by value, so the const ref is converted. assert m.roundtrip(m.get_const_ref()).x == 6 # x + 1 # by const ref, so the conversion is ok. assert m.roundtrip_const_ref(m.get_const_ref()).x == 6 + assert m.roundtrip_const_wrap(m.get_const_wrap()).is_immutable + assert m.roundtrip_const_wrap(m.get_const_ref()).is_immutable + assert m.roundtrip_const_ref(m.get_const_wrap()).is_immutable + From c747ae280e6877162f491ad7759d935127c9cd0e Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Mon, 7 Dec 2020 20:15:54 -0800 Subject: [PATCH 23/31] apply presubmit formatting --- tests/test_freezable_type_caster.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_freezable_type_caster.py b/tests/test_freezable_type_caster.py index 8b535afd2c..e113c0c398 100644 --- a/tests/test_freezable_type_caster.py +++ b/tests/test_freezable_type_caster.py @@ -91,6 +91,7 @@ def test_roundtrip(): assert m.roundtrip_const_ref(c).addr == c.addr assert m.roundtrip_const_wrap(c).addr == c.addr + def test_roundtrip_const(): m.reset(1) @@ -105,7 +106,6 @@ def test_roundtrip_const(): with pytest.raises(TypeError): m.roundtrip_wrap(m.get_const_ref()) - # by value, so the const ref is converted. assert m.roundtrip(m.get_const_ref()).x == 6 # x + 1 @@ -114,4 +114,3 @@ def test_roundtrip_const(): assert m.roundtrip_const_wrap(m.get_const_wrap()).is_immutable assert m.roundtrip_const_wrap(m.get_const_ref()).is_immutable assert m.roundtrip_const_ref(m.get_const_wrap()).is_immutable - From 33f57cc6924d360fae9d825d713830e401afdd56 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Fri, 11 Dec 2020 18:15:49 -0800 Subject: [PATCH 24/31] Revert inclusion of test_freezable_type_caster There's some concern that this test is a bit unwieldly because of the use of the raw functions. Removing for now. --- tests/CMakeLists.txt | 1 - tests/test_freezable_type_caster.cpp | 432 --------------------------- tests/test_freezable_type_caster.py | 116 ------- 3 files changed, 549 deletions(-) delete mode 100644 tests/test_freezable_type_caster.cpp delete mode 100644 tests/test_freezable_type_caster.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 611aae60fc..dae8b5ad43 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -102,7 +102,6 @@ set(PYBIND11_TEST_FILES test_eval.cpp test_exceptions.cpp test_factory_constructors.cpp - test_freezable_type_caster.cpp test_gil_scoped.cpp test_iostream.cpp test_kwargs_and_defaults.cpp diff --git a/tests/test_freezable_type_caster.cpp b/tests/test_freezable_type_caster.cpp deleted file mode 100644 index 6f9678a475..0000000000 --- a/tests/test_freezable_type_caster.cpp +++ /dev/null @@ -1,432 +0,0 @@ -/* - tests/test_freezable_type_caster.cpp - - This test verifies that const-ness is propagated through the type_caster framework. - The returned FreezableInt type is a custom type that is frozen when returned as a - const T or std::reference, and not frozen otherwise. - This test is somewhat complicated because it introduces a custom python type to - manage the frozen aspect of the type rather then relying on native pybind11 which - does not support such a feature. - - All rights reserved. Use of this source code is governed by a - BSD-style license that can be found in the LICENSE file. -*/ -#include - -#include -#include -#include -#include -#include -#include - -#include "pybind11_tests.h" - -/// C++ type -class FreezableInt { - public: - int64_t x = 1; -}; - -/// Python wrapper for C++ type which supports mutable/immutable variants. -typedef struct PyFreezableInt { - PyObject_HEAD - - FreezableInt* value; - - bool is_immutable; - bool is_owned; -} PyFreezableInt; - -PyObject* PyFreezableInt_getattro(PyObject* self, PyObject* name) { - auto* freezable = reinterpret_cast(self); - assert(freezable != nullptr); - - const char* attr = nullptr; - if (PyBytes_Check(name)) { - attr = PyBytes_AsString(name); - } -#if PY_VERSION_HEX > 0x03030000 - if (PyUnicode_Check(name)) { - attr = PyUnicode_AsUTF8(name); - } -#endif - if (attr != nullptr && strcmp(attr, "x") == 0) { - return PyLong_FromLongLong(static_cast(freezable->value->x)); - } - if (attr != nullptr && strcmp(attr, "is_immutable") == 0) { - return PyBool_FromLong((freezable->is_immutable) ? 1 : 0); - } - if (attr != nullptr && strcmp(attr, "addr") == 0) { - return PyLong_FromLongLong(reinterpret_cast(freezable->value)); - } - return PyObject_GenericGetAttr(self, name); -} - -int PyFreezableInt_setattro(PyObject* self, PyObject* name, PyObject* value) { - auto* freezable = reinterpret_cast(self); - assert(freezable != nullptr); - - const char* attr = nullptr; - if (PyBytes_Check(name)) { - attr = PyBytes_AsString(name); - } -#if PY_VERSION_HEX > 0x03030000 - if (PyUnicode_Check(name)) { - attr = PyUnicode_AsUTF8(name); - } -#endif - if (attr != nullptr && strcmp(attr, "x") == 0) { - if (freezable->is_immutable) { - PyErr_Format(PyExc_ValueError, "Instance is immutable; cannot set values"); - return -1; - } - - if (PyLong_Check(value)) { - freezable->value->x = static_cast(PyLong_AsLongLong(value)); - return 0; - } -#if PY_VERSION_HEX < 0x03000000 - if (PyInt_Check(value)) { - freezable->value->x = static_cast(PyInt_AsLong(value)); - return 0; - } -#endif - // "Cannot set a non-numeric value of type %s" - PyErr_SetObject(PyExc_ValueError, value); - return -1; - } - - return PyObject_GenericSetAttr(self, name, value); -} - -void PyFreezableInt_dealloc(PyObject* self); - - -/* https://github.com/cython/cython/issues/3474 */ -#if defined(__GNUC__) || defined(__clang__) && PY_VERSION_HEX >= 0x030800b4 && PY_VERSION_HEX < 0x03090000 -#define PY38_WARNING_WORKAROUND_ENABLED 1 -#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Wdeprecated-declarations" -#endif - -static PyTypeObject PyFreezableInt_Type{ - PyVarObject_HEAD_INIT(nullptr, 0) /**/ - "pybind11_tests.test_freezable.FreezableInt", /* tp_name */ - sizeof(PyFreezableInt), /* tp_basicsize */ - 0, /* tp_itemsize */ - &PyFreezableInt_dealloc, /* tp_dealloc */ - 0, /* tp_print | tp_vectorcall_offset */ - 0, /* tp_getattr */ - 0, /* tp_setattr */ - 0, /* tp_as_async */ - 0, /* tp_repr */ - 0, /* tp_as_number */ - 0, /* tp_as_sequence */ - 0, /* tp_as_mapping */ - 0, /* tp_hash */ - 0, /* tp_call */ - 0, /* tp_str */ - &PyFreezableInt_getattro, /* tp_getattro */ - &PyFreezableInt_setattro, /* tp_setattro */ - 0, /* tp_as_buffer */ - Py_TPFLAGS_DEFAULT, /* tp_flags */ - "FreezableInt objects", /* tp_doc */ - 0, /* tp_traverse */ - 0, /* tp_clear */ - 0, /* tp_richcompare */ - 0, /* tp_weaklistoffset */ - 0, /* tp_iter */ - 0, /* tp_iternext */ - 0, /* tp_methods */ - 0, /* tp_members */ - 0, /* tp_getset */ - 0, /* tp_base */ - 0, /* tp_dict */ - 0, /* tp_descr_get */ - 0, /* tp_descr_set */ - 0, /* tp_dictoffset */ - 0, /* tp_init */ - 0, /* tp_alloc */ - 0, /* tp_new */ - 0, /* tp_free */ - 0, /* tp_is_gc */ - 0, /* tp_bases */ - 0, /* tp_mro */ - 0, /* tp_cache */ - 0, /* tp_subclasses */ - 0, /* tp_weaklist */ - 0, /* tp_del */ - 0, /* tp_version_tag */ -#if PY_VERSION_HEX >= 0x030400a1 - 0, /* tp_finalize */ -#endif -#if PY_VERSION_HEX >= 0x030800b1 - 0, /* tp_vectorcall */ -#endif -#if PY_VERSION_HEX >= 0x030800b4 && PY_VERSION_HEX < 0x03090000 - 0, /* tp_print */ -#endif -#if defined(PYPY_VERSION) - 0, /* tp_pypy_flags */ -#endif -}; - - -#if defined(PY38_WARNING_WORKAROUND_ENABLED) -#undef PY38_WARNING_WORKAROUND_ENABLED -#pragma GCC diagnostic pop -#endif - -static std::unordered_map mapping; - -void PyFreezableInt_dealloc(PyObject* self) { - auto* freezable = reinterpret_cast(self); - auto it = mapping.find(freezable->value); - if (it != mapping.end()) { - mapping.erase(it); - } - if (freezable->is_owned) { - delete freezable->value; - } - Py_TYPE(self)->tp_free(self); -} - -PyObject* PyFreezableInt_new(FreezableInt* value, bool is_immutable) { - if (PyType_Ready(&PyFreezableInt_Type) != 0) { - return nullptr; - } - PyFreezableInt* self = PyObject_New(PyFreezableInt, &PyFreezableInt_Type); - if (!self) { - return nullptr; - } - mapping.emplace(value, self); - self->value = value; - self->is_owned = false; - self->is_immutable = is_immutable; - return reinterpret_cast(self); -} - -PyObject* PyFreezableInt_new(std::unique_ptr value) { - PyObject* self = PyFreezableInt_new(value.get(), false); - if (self != nullptr) { - auto* freezable = reinterpret_cast(self); - freezable->value = value.release(); - freezable->is_owned = true; - } - return self; -} - - -PyObject* PyFreezableInt_reference(FreezableInt* value, bool is_immutable) { - auto it = mapping.find(value); - if (it != mapping.end()) { - auto* freezable = static_cast(it->second); - if (!is_immutable) { - // We have a single cache of C++ pointer to python object, so if any of the - // objects becomes immutable, they all become immutable. - freezable->is_immutable = false; - } - Py_INCREF(freezable); - return reinterpret_cast(freezable); - } - return PyFreezableInt_new(value, is_immutable); -} - -/// pybind11 type_caster which returns custom PyFreezableInt python wrapper instead -/// of a pybind11 generated type; this is used to make const* and const& immutable -/// in python. -namespace pybind11 { -namespace detail { - -template <> -struct type_caster { - static constexpr auto name = _(); - - // C++ -> Python - // ... construct an immutable python type referencing src. This isn't really - // available in pybind11 and would have to be built by hand. - static handle cast(const FreezableInt* src, return_value_policy, handle) { - if (!src) return pybind11::none().release(); - return PyFreezableInt_reference(const_cast(src), true); - } - static handle cast(const FreezableInt& src, return_value_policy policy, - handle parent) { - return cast(&src, policy, parent); - } - - // ... construct a mutable python type referencing src. This is the default - // pybind11 path. - static handle cast(FreezableInt* src, return_value_policy, handle) { - if (!src) return pybind11::none().release(); - return PyFreezableInt_reference(src, false); - } - static handle cast(FreezableInt& src, return_value_policy policy, handle parent) { - return cast(&src, policy, parent); - } - - // ... construct a mutable python type owning src. - static handle cast(FreezableInt&& src, return_value_policy, handle) { - std::unique_ptr ptr(new FreezableInt(std::move(src))); - return PyFreezableInt_new(std::move(ptr)); - } - - type_caster() = default; - type_caster(type_caster&&) = default; - type_caster(const type_caster& other) : freezable(other.freezable) { - if (freezable) Py_INCREF(freezable); - } - - type_caster& operator=(type_caster&&) = default; - type_caster& operator=(const type_caster& other) { - if (freezable) Py_DECREF(freezable); - freezable = other.freezable; - if (freezable) Py_INCREF(freezable); - return *this; - } - - ~type_caster() { - if (freezable) Py_DECREF(freezable); - } - - // Convert Python->C++. - // ... Merely capture the PyFreezableInt pointer and do additional work in the - // conversion operator. - bool load(handle src, bool) { - if (!PyObject_TypeCheck(src.ptr(), &PyFreezableInt_Type)) { - return false; - } - assert(freezable == nullptr); - freezable = reinterpret_cast(src.ptr()); - Py_INCREF(freezable); - return true; - } - - // cast_op_type determines which operator overload to call for a given c++ - // input parameter type. In this case we want to propagate const, etc. - template - using cast_op_type = conditional_t< - std::is_same, const FreezableInt*>::value, const FreezableInt*, - conditional_t< - std::is_same, FreezableInt*>::value, FreezableInt*, - conditional_t::value, const FreezableInt&, - conditional_t::value, FreezableInt&, - conditional_t::value, FreezableInt&&, - /* by-value is a reasonable default */ FreezableInt>>>>>; - - // PYBIND11_TYPE_CASTER - operator const FreezableInt*() { return freezable->value; } - operator const FreezableInt&() { - if (!freezable || !freezable->value) throw reference_cast_error(); - return *freezable->value; - } - operator FreezableInt*() { - if (freezable->is_immutable) throw reference_cast_error(); - return freezable->value; - } - operator FreezableInt&() { - if (!freezable || !freezable->value) throw reference_cast_error(); - if (freezable->is_immutable) throw reference_cast_error(); - return *freezable->value; - } - operator FreezableInt&&() && { - if (!freezable || !freezable->value) throw reference_cast_error(); - owned = *freezable->value; - return std::move(owned); - } - operator FreezableInt() { - if (!freezable || !freezable->value) throw reference_cast_error(); - FreezableInt v = *freezable->value; - return v; - } - - protected: - PyFreezableInt* freezable = nullptr; - FreezableInt owned; -}; - -} // namespace detail -} // namespace pybind11 - -/// C++ module using pybind11 type_caster returning mutable/immutable -/// objects. - -static FreezableInt shared; // returned by non-const get. -static FreezableInt shared_const; // returned by const get. - -TEST_SUBMODULE(test_freezable_type_caster, m) { - - Py_INCREF(&PyFreezableInt_Type); - - m.def("make", []() -> FreezableInt { return FreezableInt{}; }); - m.def("reset", [](int x) { shared.x = x; shared_const.x = x; }); - - // it takes and it takes and it takes - m.def("take", [](FreezableInt c) { - c.x++; - return c.x; - }); - - m.def("take_ptr", [](FreezableInt* c) { - c->x++; - return c->x; - }); - m.def("take_ref", [](FreezableInt& c) { - c.x++; - return c.x; - }); - m.def("take_wrap", [](std::reference_wrapper c) { - c.get().x++; - return c.get().x; - }); - - m.def("take_const_ptr", [](const FreezableInt* c) { return 10 * c->x; }); - m.def("take_const_ref", [](const FreezableInt& c) { return 10 * c.x; }); - m.def("take_const_wrap", - [](std::reference_wrapper c) { return 10 * c.get().x; }); - - m.def("get", []() -> FreezableInt { return shared; }); - - m.def("get_ptr", []() -> FreezableInt* { return &shared; }); - m.def("get_ref", []() -> FreezableInt& { return shared; }); - m.def("get_wrap", - []() -> std::reference_wrapper { return std::ref(shared); }); - - m.def("get_const_ptr", []() -> const FreezableInt* { - shared_const.x++; - return &shared_const; - }); - m.def("get_const_ref", []() -> const FreezableInt& { - shared_const.x++; - return shared_const; - }); - m.def("get_const_wrap", []() -> std::reference_wrapper { - shared_const.x++; - return std::cref(shared_const); - }); - - m.def("roundtrip", [](FreezableInt c) -> FreezableInt { - c.x++; - return c; - }); - m.def("roundtrip_ptr", [](FreezableInt* c) -> FreezableInt* { - c->x++; - return c; - }); - m.def("roundtrip_ref", [](FreezableInt& c) -> FreezableInt& { - c.x++; - return c; - }); - m.def("roundtrip_wrap", - [](std::reference_wrapper c) -> std::reference_wrapper { - c.get().x++; - return c; - }); - m.def("roundtrip_const_ref", [](const FreezableInt& c) -> const FreezableInt& { - return c; - }); - m.def("roundtrip_const_wrap", [](std::reference_wrapper c) -> std::reference_wrapper { - return c; - }); - -} diff --git a/tests/test_freezable_type_caster.py b/tests/test_freezable_type_caster.py deleted file mode 100644 index e113c0c398..0000000000 --- a/tests/test_freezable_type_caster.py +++ /dev/null @@ -1,116 +0,0 @@ -# -*- coding: utf-8 -*- -import pytest - -import env # noqa: F401 - -from pybind11_tests import test_freezable_type_caster as m - - -def test_make(): - assert m.make().x == 1 - - -def test_take(): - c = m.make() - assert m.take(c) == 2 - assert m.take(c) == 2 - assert m.take_ptr(c) == 2 - assert m.take_ptr(c) == 3 - assert m.take_ref(c) == 4 - assert m.take_ref(c) == 5 - assert m.take_wrap(c) == 6 - assert m.take_wrap(c) == 7 - - assert m.take_const_ptr(c) == 70 - assert m.take_const_ref(c) == 70 - assert m.take_const_wrap(c) == 70 - - -def test_get(): - """The get calls all return a variant of the same shared pointer.""" - m.reset(1) - assert m.get().x == 1 - v = m.get_ptr() - assert v.x == 1 - assert not v.is_immutable - v.x = 10 - assert m.get_ptr().x == 10 - v.x = 11 - assert m.get_ref().x == 11 - v.x = 12 - assert m.get_wrap().x == 12 - v.x = 13 - assert m.get().x == 13 - - -def test_get_const(): - m.reset(1) - v = m.get_const_ptr() - assert v.x == 2 - assert v.is_immutable - assert m.get_const_ref().x == 3 - assert m.get_const_wrap().x == 4 - assert m.get_const_ptr().x == 5 - assert m.get_const_ref().x == 6 - assert m.get_const_wrap().x == 7 - assert v.x == 7 - - assert m.get_const_ref().is_immutable - assert m.get_const_wrap().is_immutable - assert m.get_const_ptr().is_immutable - - assert m.get_const_wrap().addr == m.get_const_ref().addr - assert m.get_const_wrap().addr == m.get_const_ptr().addr - assert m.get_const_ref().addr == m.get_const_ptr().addr - - # immutable - with pytest.raises(ValueError): - v.x = 1 - with pytest.raises(ValueError): - m.get_const_ref().x = 1 - with pytest.raises(ValueError): - m.get_const_wrap().x = 1 - with pytest.raises(ValueError): - m.get_const_ptr().x = 1 - - -def test_roundtrip(): - c = m.make() - assert c.x == 1 - assert m.roundtrip(c).x == 2 - assert m.roundtrip(c).x == 2 - - assert m.roundtrip_ptr(c).x == 2 - assert m.roundtrip_ref(c).x == 3 - assert m.roundtrip_wrap(c).x == 4 - - assert m.roundtrip_ptr(m.get_ref()).x == 2 - assert m.roundtrip_ref(m.get_wrap()).x == 3 - assert m.roundtrip_wrap(m.get_ptr()).x == 4 - - assert m.roundtrip_const_ref(c).addr == c.addr - assert m.roundtrip_const_wrap(c).addr == c.addr - - -def test_roundtrip_const(): - m.reset(1) - - # NOTE: each call to get_const_ref increments x. - # by reference, the const ref is not converted. - with pytest.raises(TypeError): - m.roundtrip_ref(m.get_const_ref()) - - with pytest.raises(TypeError): - m.roundtrip_ptr(m.get_const_ref()) - - with pytest.raises(TypeError): - m.roundtrip_wrap(m.get_const_ref()) - - # by value, so the const ref is converted. - assert m.roundtrip(m.get_const_ref()).x == 6 # x + 1 - - # by const ref, so the conversion is ok. - assert m.roundtrip_const_ref(m.get_const_ref()).x == 6 - assert m.roundtrip_const_wrap(m.get_const_wrap()).is_immutable - assert m.roundtrip_const_wrap(m.get_const_ref()).is_immutable - assert m.roundtrip_const_ref(m.get_const_wrap()).is_immutable From d869976b275be1bb9c15245b6de854613006b3d0 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Mon, 14 Dec 2020 11:44:51 -0800 Subject: [PATCH 25/31] Add a test that validates const references propagation. This test verifies that cast_op may be used to correctly detect const reference types when used with std::reference_wrapper. --- tests/CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index dae8b5ad43..89d19f6fc4 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -93,6 +93,7 @@ set(PYBIND11_TEST_FILES test_callbacks.cpp test_chrono.cpp test_class.cpp + test_const_ref_caster.cpp test_constants_and_functions.cpp test_copy_move.cpp test_custom_type_casters.cpp From f109eddda8c14809e680cd16aa38286a16f33720 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Mon, 14 Dec 2020 11:47:15 -0800 Subject: [PATCH 26/31] mend --- tests/test_const_ref_caster.cpp | 88 +++++++++++++++++++++++++++++++++ tests/test_const_ref_caster.py | 18 +++++++ 2 files changed, 106 insertions(+) create mode 100644 tests/test_const_ref_caster.cpp create mode 100644 tests/test_const_ref_caster.py diff --git a/tests/test_const_ref_caster.cpp b/tests/test_const_ref_caster.cpp new file mode 100644 index 0000000000..8b56f600cc --- /dev/null +++ b/tests/test_const_ref_caster.cpp @@ -0,0 +1,88 @@ +/* + tests/test_const_ref_caster.cpp + + This test verifies that const-ref is propagated through type_caster cast_op. + + The returned ConstRefCasted type is a mimimal type that is constructed to + reference the casting mode used. +*/ + +#include + +#include "pybind11_tests.h" + + +struct ConstRefCasted { + bool is_const; + bool is_ref; +}; + +PYBIND11_NAMESPACE_BEGIN(pybind11) +PYBIND11_NAMESPACE_BEGIN(detail) +template <> +class type_caster { + public: + static constexpr auto name = _(); + + bool load(handle, bool) { return true; } + + operator ConstRefCasted&&() { value = {false, false}; return std::move(value); } + operator ConstRefCasted&() { value = {false, true}; return value; } + operator ConstRefCasted*() { value = {false, false}; return &value; } + + operator const ConstRefCasted&() { value = {true, true}; return value; } + operator const ConstRefCasted*() { value = {true, false}; return &value; } + + // + template + using cast_op_type = + /// const + conditional_t< + std::is_same, const ConstRefCasted*>::value, const ConstRefCasted*, + conditional_t< + std::is_same::value, const ConstRefCasted&, + // non-const + conditional_t< + std::is_same, ConstRefCasted*>::value, ConstRefCasted*, + conditional_t< + std::is_same::value, ConstRefCasted&, + /* else */ConstRefCasted&&>>>>; + + private: + ConstRefCasted value = {false, false}; +}; +PYBIND11_NAMESPACE_END(detail) +PYBIND11_NAMESPACE_END(pybind11) + +TEST_SUBMODULE(const_ref_caster, m) { + py::class_(m, "ConstRefCasted", + "A `py::class_` type for testing") + .def(py::init<>()) + .def_readonly("is_const", &ConstRefCasted::is_const) + .def_readonly("is_ref", &ConstRefCasted::is_ref); + + + m.def("takes", [](ConstRefCasted x) { + return !x.is_const && !x.is_ref; + }); + m.def("takes_ptr", [](ConstRefCasted* x) { + return !x->is_const && !x->is_ref; + }); + m.def("takes_ref", [](ConstRefCasted& x) { + return !x.is_const && x.is_ref; + }); + m.def("takes_ref_wrap", [](std::reference_wrapper x) { + return !x.get().is_const && x.get().is_ref; + }); + + m.def("takes_const_ptr", [](const ConstRefCasted* x) { + return x->is_const && !x->is_ref; + }); + m.def("takes_const_ref", [](const ConstRefCasted& x) { + return x.is_const && x.is_ref; + }); + m.def("takes_const_ref_wrap", + [](std::reference_wrapper x) { + return x.get().is_const && x.get().is_ref; + }); +} diff --git a/tests/test_const_ref_caster.py b/tests/test_const_ref_caster.py new file mode 100644 index 0000000000..9702e50068 --- /dev/null +++ b/tests/test_const_ref_caster.py @@ -0,0 +1,18 @@ +# -*- coding: utf-8 -*- +import pytest + +import env # noqa: F401 + +from pybind11_tests import const_ref_caster as m + + +def test_takes(): + assert m.takes(m.ConstRefCasted()) + + assert m.takes_ptr(m.ConstRefCasted()) + assert m.takes_ref(m.ConstRefCasted()) + assert m.takes_ref_wrap(m.ConstRefCasted()) + + assert m.takes_const_ptr(m.ConstRefCasted()) + assert m.takes_const_ref(m.ConstRefCasted()) + assert m.takes_const_ref_wrap(m.ConstRefCasted()) From 775a7c90d75b2fbe0f80d0416659c4ef81400cb5 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Mon, 14 Dec 2020 13:02:36 -0800 Subject: [PATCH 27/31] Review comments based changes. 1. std::add_lvalue_reference -> type& 2. Simplify the test a little more; we're never returning the ConstRefCaster type so the class_ definition can be removed. --- include/pybind11/cast.h | 13 ++++++------- tests/test_const_ref_caster.cpp | 12 +++--------- tests/test_const_ref_caster.py | 15 ++++++++------- 3 files changed, 17 insertions(+), 23 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 23a7a43df3..89f9fa9074 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -960,15 +960,14 @@ template class type_caster> { private: using caster_t = make_caster; caster_t subcaster; - using reference_t = typename std::add_lvalue_reference::type; + using reference_t = type&; using subcaster_cast_op_type = typename caster_t::template cast_op_type; - static_assert((std::is_same::type &, - subcaster_cast_op_type>::value || - std::is_same::value), - "std::reference_wrapper caster requires T to have a caster " - "with an operator `T &` or `const T&`"); + static_assert(std::is_same::type &, subcaster_cast_op_type>::value || + std::is_same::value, + "std::reference_wrapper caster requires T to have a caster with an " + "`operator T &()` or `operator const T &()`"); public: bool load(handle src, bool convert) { return subcaster.load(src, convert); } static constexpr auto name = caster_t::name; @@ -979,7 +978,7 @@ template class type_caster> { return caster_t::cast(&src.get(), policy, parent); } template using cast_op_type = std::reference_wrapper; - operator std::reference_wrapper() { return subcaster.operator subcaster_cast_op_type(); } + operator std::reference_wrapper() { return subcaster.operator subcaster_cast_op_type(); } }; #define PYBIND11_TYPE_CASTER(type, py_name) \ diff --git a/tests/test_const_ref_caster.cpp b/tests/test_const_ref_caster.cpp index 8b56f600cc..7089889dca 100644 --- a/tests/test_const_ref_caster.cpp +++ b/tests/test_const_ref_caster.cpp @@ -7,7 +7,7 @@ reference the casting mode used. */ -#include +#include #include "pybind11_tests.h" @@ -33,7 +33,7 @@ class type_caster { operator const ConstRefCasted&() { value = {true, true}; return value; } operator const ConstRefCasted*() { value = {true, false}; return &value; } - // + // custom cast_op to explicitly propagate types to the conversion operators. template using cast_op_type = /// const @@ -41,7 +41,7 @@ class type_caster { std::is_same, const ConstRefCasted*>::value, const ConstRefCasted*, conditional_t< std::is_same::value, const ConstRefCasted&, - // non-const + /// non-const conditional_t< std::is_same, ConstRefCasted*>::value, ConstRefCasted*, conditional_t< @@ -55,12 +55,6 @@ PYBIND11_NAMESPACE_END(detail) PYBIND11_NAMESPACE_END(pybind11) TEST_SUBMODULE(const_ref_caster, m) { - py::class_(m, "ConstRefCasted", - "A `py::class_` type for testing") - .def(py::init<>()) - .def_readonly("is_const", &ConstRefCasted::is_const) - .def_readonly("is_ref", &ConstRefCasted::is_ref); - m.def("takes", [](ConstRefCasted x) { return !x.is_const && !x.is_ref; diff --git a/tests/test_const_ref_caster.py b/tests/test_const_ref_caster.py index 9702e50068..327a94991a 100644 --- a/tests/test_const_ref_caster.py +++ b/tests/test_const_ref_caster.py @@ -7,12 +7,13 @@ def test_takes(): - assert m.takes(m.ConstRefCasted()) + x = False + assert m.takes(x) - assert m.takes_ptr(m.ConstRefCasted()) - assert m.takes_ref(m.ConstRefCasted()) - assert m.takes_ref_wrap(m.ConstRefCasted()) + assert m.takes_ptr(x) + assert m.takes_ref(x) + assert m.takes_ref_wrap(x) - assert m.takes_const_ptr(m.ConstRefCasted()) - assert m.takes_const_ref(m.ConstRefCasted()) - assert m.takes_const_ref_wrap(m.ConstRefCasted()) + assert m.takes_const_ptr(x) + assert m.takes_const_ref(x) + assert m.takes_const_ref_wrap(x) From 6f6dac75bd20c5a1243322d6562c631087a469b4 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Mon, 14 Dec 2020 13:06:04 -0800 Subject: [PATCH 28/31] formatted files again. --- include/pybind11/cast.h | 2 +- tests/test_const_ref_caster.py | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 89f9fa9074..ba55a33a9e 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -978,7 +978,7 @@ template class type_caster> { return caster_t::cast(&src.get(), policy, parent); } template using cast_op_type = std::reference_wrapper; - operator std::reference_wrapper() { return subcaster.operator subcaster_cast_op_type(); } + operator std::reference_wrapper() { return subcaster.operator subcaster_cast_op_type(); } }; #define PYBIND11_TYPE_CASTER(type, py_name) \ diff --git a/tests/test_const_ref_caster.py b/tests/test_const_ref_caster.py index 327a94991a..a8c7a6f928 100644 --- a/tests/test_const_ref_caster.py +++ b/tests/test_const_ref_caster.py @@ -7,13 +7,13 @@ def test_takes(): - x = False - assert m.takes(x) + x = False + assert m.takes(x) - assert m.takes_ptr(x) - assert m.takes_ref(x) - assert m.takes_ref_wrap(x) + assert m.takes_ptr(x) + assert m.takes_ref(x) + assert m.takes_ref_wrap(x) - assert m.takes_const_ptr(x) - assert m.takes_const_ref(x) - assert m.takes_const_ref_wrap(x) + assert m.takes_const_ptr(x) + assert m.takes_const_ref(x) + assert m.takes_const_ref_wrap(x) From fd6dc5b044788cf26ba1cc7faa555b7a08338bdf Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Mon, 14 Dec 2020 14:39:18 -0800 Subject: [PATCH 29/31] Move const_ref_caster test to builtin_casters --- tests/CMakeLists.txt | 1 - tests/test_builtin_casters.cpp | 52 +++++++++++++++++++++ tests/test_builtin_casters.py | 17 +++++++ tests/test_const_ref_caster.cpp | 82 --------------------------------- tests/test_const_ref_caster.py | 19 -------- 5 files changed, 69 insertions(+), 102 deletions(-) delete mode 100644 tests/test_const_ref_caster.cpp delete mode 100644 tests/test_const_ref_caster.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 89d19f6fc4..dae8b5ad43 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -93,7 +93,6 @@ set(PYBIND11_TEST_FILES test_callbacks.cpp test_chrono.cpp test_class.cpp - test_const_ref_caster.cpp test_constants_and_functions.cpp test_copy_move.cpp test_custom_type_casters.cpp diff --git a/tests/test_builtin_casters.cpp b/tests/test_builtin_casters.cpp index 8fd5b73199..0649b91e90 100644 --- a/tests/test_builtin_casters.cpp +++ b/tests/test_builtin_casters.cpp @@ -15,6 +15,49 @@ # pragma warning(disable: 4127) // warning C4127: Conditional expression is constant #endif +struct ConstRefCasted { + bool is_const; + bool is_ref; +}; + + +PYBIND11_NAMESPACE_BEGIN(pybind11) +PYBIND11_NAMESPACE_BEGIN(detail) +template <> +class type_caster { + public: + static constexpr auto name = _(); + + bool load(handle, bool) { return true; } + + operator ConstRefCasted&&() { value = {false, false}; return std::move(value); } + operator ConstRefCasted&() { value = {false, true}; return value; } + operator ConstRefCasted*() { value = {false, false}; return &value; } + + operator const ConstRefCasted&() { value = {true, true}; return value; } + operator const ConstRefCasted*() { value = {true, false}; return &value; } + + // custom cast_op to explicitly propagate types to the conversion operators. + template + using cast_op_type = + /// const + conditional_t< + std::is_same, const ConstRefCasted*>::value, const ConstRefCasted*, + conditional_t< + std::is_same::value, const ConstRefCasted&, + /// non-const + conditional_t< + std::is_same, ConstRefCasted*>::value, ConstRefCasted*, + conditional_t< + std::is_same::value, ConstRefCasted&, + /* else */ConstRefCasted&&>>>>; + + private: + ConstRefCasted value = {false, false}; +}; +PYBIND11_NAMESPACE_END(detail) +PYBIND11_NAMESPACE_END(pybind11) + TEST_SUBMODULE(builtin_casters, m) { // test_simple_string m.def("string_roundtrip", [](const char *s) { return s; }); @@ -200,4 +243,13 @@ TEST_SUBMODULE(builtin_casters, m) { py::object o = py::cast(v); return py::cast(o) == v; }); + + // Tests const/non-const propagation in cast_op + m.def("takes", [](ConstRefCasted x) { return !x.is_const && !x.is_ref; }); + m.def("takes_ptr", [](ConstRefCasted* x) { return !x->is_const && !x->is_ref; }); + m.def("takes_ref", [](ConstRefCasted& x) { return !x.is_const && x.is_ref; }); + m.def("takes_ref_wrap", [](std::reference_wrapper x) { return !x.get().is_const && x.get().is_ref; }); + m.def("takes_const_ptr", [](const ConstRefCasted* x) { return x->is_const && !x->is_ref; }); + m.def("takes_const_ref", [](const ConstRefCasted& x) { return x.is_const && x.is_ref; }); + m.def("takes_const_ref_wrap", [](std::reference_wrapper x) { return x.get().is_const && x.get().is_ref; }); } diff --git a/tests/test_builtin_casters.py b/tests/test_builtin_casters.py index 99cdae29b6..735663b54e 100644 --- a/tests/test_builtin_casters.py +++ b/tests/test_builtin_casters.py @@ -425,3 +425,20 @@ def test_int_long(): def test_void_caster_2(): assert m.test_void_caster() + + +def test_const_ref_caster(): + """Verifies that const-ref is propagated through type_caster cast_op. + The returned ConstRefCasted type is a mimimal type that is constructed to + reference the casting mode used. + """ + x = False + assert m.takes(x) + + assert m.takes_ptr(x) + assert m.takes_ref(x) + assert m.takes_ref_wrap(x) + + assert m.takes_const_ptr(x) + assert m.takes_const_ref(x) + assert m.takes_const_ref_wrap(x) diff --git a/tests/test_const_ref_caster.cpp b/tests/test_const_ref_caster.cpp deleted file mode 100644 index 7089889dca..0000000000 --- a/tests/test_const_ref_caster.cpp +++ /dev/null @@ -1,82 +0,0 @@ -/* - tests/test_const_ref_caster.cpp - - This test verifies that const-ref is propagated through type_caster cast_op. - - The returned ConstRefCasted type is a mimimal type that is constructed to - reference the casting mode used. -*/ - -#include - -#include "pybind11_tests.h" - - -struct ConstRefCasted { - bool is_const; - bool is_ref; -}; - -PYBIND11_NAMESPACE_BEGIN(pybind11) -PYBIND11_NAMESPACE_BEGIN(detail) -template <> -class type_caster { - public: - static constexpr auto name = _(); - - bool load(handle, bool) { return true; } - - operator ConstRefCasted&&() { value = {false, false}; return std::move(value); } - operator ConstRefCasted&() { value = {false, true}; return value; } - operator ConstRefCasted*() { value = {false, false}; return &value; } - - operator const ConstRefCasted&() { value = {true, true}; return value; } - operator const ConstRefCasted*() { value = {true, false}; return &value; } - - // custom cast_op to explicitly propagate types to the conversion operators. - template - using cast_op_type = - /// const - conditional_t< - std::is_same, const ConstRefCasted*>::value, const ConstRefCasted*, - conditional_t< - std::is_same::value, const ConstRefCasted&, - /// non-const - conditional_t< - std::is_same, ConstRefCasted*>::value, ConstRefCasted*, - conditional_t< - std::is_same::value, ConstRefCasted&, - /* else */ConstRefCasted&&>>>>; - - private: - ConstRefCasted value = {false, false}; -}; -PYBIND11_NAMESPACE_END(detail) -PYBIND11_NAMESPACE_END(pybind11) - -TEST_SUBMODULE(const_ref_caster, m) { - - m.def("takes", [](ConstRefCasted x) { - return !x.is_const && !x.is_ref; - }); - m.def("takes_ptr", [](ConstRefCasted* x) { - return !x->is_const && !x->is_ref; - }); - m.def("takes_ref", [](ConstRefCasted& x) { - return !x.is_const && x.is_ref; - }); - m.def("takes_ref_wrap", [](std::reference_wrapper x) { - return !x.get().is_const && x.get().is_ref; - }); - - m.def("takes_const_ptr", [](const ConstRefCasted* x) { - return x->is_const && !x->is_ref; - }); - m.def("takes_const_ref", [](const ConstRefCasted& x) { - return x.is_const && x.is_ref; - }); - m.def("takes_const_ref_wrap", - [](std::reference_wrapper x) { - return x.get().is_const && x.get().is_ref; - }); -} diff --git a/tests/test_const_ref_caster.py b/tests/test_const_ref_caster.py deleted file mode 100644 index a8c7a6f928..0000000000 --- a/tests/test_const_ref_caster.py +++ /dev/null @@ -1,19 +0,0 @@ -# -*- coding: utf-8 -*- -import pytest - -import env # noqa: F401 - -from pybind11_tests import const_ref_caster as m - - -def test_takes(): - x = False - assert m.takes(x) - - assert m.takes_ptr(x) - assert m.takes_ref(x) - assert m.takes_ref_wrap(x) - - assert m.takes_const_ptr(x) - assert m.takes_const_ref(x) - assert m.takes_const_ref_wrap(x) From b3ecc530cab73cc3bc158971a283729c2343128b Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Mon, 14 Dec 2020 16:25:03 -0800 Subject: [PATCH 30/31] Review comments: use cast_op and adjust some comments. --- include/pybind11/cast.h | 2 +- tests/test_builtin_casters.cpp | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index ba55a33a9e..d2869264c3 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -978,7 +978,7 @@ template class type_caster> { return caster_t::cast(&src.get(), policy, parent); } template using cast_op_type = std::reference_wrapper; - operator std::reference_wrapper() { return subcaster.operator subcaster_cast_op_type(); } + operator std::reference_wrapper() { return cast_op(subcaster); } }; #define PYBIND11_TYPE_CASTER(type, py_name) \ diff --git a/tests/test_builtin_casters.cpp b/tests/test_builtin_casters.cpp index 0649b91e90..5727071a91 100644 --- a/tests/test_builtin_casters.cpp +++ b/tests/test_builtin_casters.cpp @@ -20,7 +20,6 @@ struct ConstRefCasted { bool is_ref; }; - PYBIND11_NAMESPACE_BEGIN(pybind11) PYBIND11_NAMESPACE_BEGIN(detail) template <> @@ -28,6 +27,8 @@ class type_caster { public: static constexpr auto name = _(); + // Input is unimportant, a new value will always be constructed based on the + // cast operator. bool load(handle, bool) { return true; } operator ConstRefCasted&&() { value = {false, false}; return std::move(value); } @@ -244,7 +245,7 @@ TEST_SUBMODULE(builtin_casters, m) { return py::cast(o) == v; }); - // Tests const/non-const propagation in cast_op + // Tests const/non-const propagation in cast_op. m.def("takes", [](ConstRefCasted x) { return !x.is_const && !x.is_ref; }); m.def("takes_ptr", [](ConstRefCasted* x) { return !x->is_const && !x->is_ref; }); m.def("takes_ref", [](ConstRefCasted& x) { return !x.is_const && x.is_ref; }); From c22772b75d6d6dae413b46bc4dbcec929a938339 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Mon, 14 Dec 2020 17:45:58 -0800 Subject: [PATCH 31/31] Simplify ConstRefCasted test I like this version better as it moves the assertion that matters back into python. --- tests/test_builtin_casters.cpp | 30 +++++++++++++++--------------- tests/test_builtin_casters.py | 15 ++++++++------- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/tests/test_builtin_casters.cpp b/tests/test_builtin_casters.cpp index 5727071a91..e16c2d62bc 100644 --- a/tests/test_builtin_casters.cpp +++ b/tests/test_builtin_casters.cpp @@ -16,8 +16,7 @@ #endif struct ConstRefCasted { - bool is_const; - bool is_ref; + int tag; }; PYBIND11_NAMESPACE_BEGIN(pybind11) @@ -31,12 +30,12 @@ class type_caster { // cast operator. bool load(handle, bool) { return true; } - operator ConstRefCasted&&() { value = {false, false}; return std::move(value); } - operator ConstRefCasted&() { value = {false, true}; return value; } - operator ConstRefCasted*() { value = {false, false}; return &value; } + operator ConstRefCasted&&() { value = {1}; return std::move(value); } + operator ConstRefCasted&() { value = {2}; return value; } + operator ConstRefCasted*() { value = {3}; return &value; } - operator const ConstRefCasted&() { value = {true, true}; return value; } - operator const ConstRefCasted*() { value = {true, false}; return &value; } + operator const ConstRefCasted&() { value = {4}; return value; } + operator const ConstRefCasted*() { value = {5}; return &value; } // custom cast_op to explicitly propagate types to the conversion operators. template @@ -54,7 +53,7 @@ class type_caster { /* else */ConstRefCasted&&>>>>; private: - ConstRefCasted value = {false, false}; + ConstRefCasted value = {0}; }; PYBIND11_NAMESPACE_END(detail) PYBIND11_NAMESPACE_END(pybind11) @@ -246,11 +245,12 @@ TEST_SUBMODULE(builtin_casters, m) { }); // Tests const/non-const propagation in cast_op. - m.def("takes", [](ConstRefCasted x) { return !x.is_const && !x.is_ref; }); - m.def("takes_ptr", [](ConstRefCasted* x) { return !x->is_const && !x->is_ref; }); - m.def("takes_ref", [](ConstRefCasted& x) { return !x.is_const && x.is_ref; }); - m.def("takes_ref_wrap", [](std::reference_wrapper x) { return !x.get().is_const && x.get().is_ref; }); - m.def("takes_const_ptr", [](const ConstRefCasted* x) { return x->is_const && !x->is_ref; }); - m.def("takes_const_ref", [](const ConstRefCasted& x) { return x.is_const && x.is_ref; }); - m.def("takes_const_ref_wrap", [](std::reference_wrapper x) { return x.get().is_const && x.get().is_ref; }); + m.def("takes", [](ConstRefCasted x) { return x.tag; }); + m.def("takes_move", [](ConstRefCasted&& x) { return x.tag; }); + m.def("takes_ptr", [](ConstRefCasted* x) { return x->tag; }); + m.def("takes_ref", [](ConstRefCasted& x) { return x.tag; }); + m.def("takes_ref_wrap", [](std::reference_wrapper x) { return x.get().tag; }); + m.def("takes_const_ptr", [](const ConstRefCasted* x) { return x->tag; }); + m.def("takes_const_ref", [](const ConstRefCasted& x) { return x.tag; }); + m.def("takes_const_ref_wrap", [](std::reference_wrapper x) { return x.get().tag; }); } diff --git a/tests/test_builtin_casters.py b/tests/test_builtin_casters.py index 735663b54e..39e8711dff 100644 --- a/tests/test_builtin_casters.py +++ b/tests/test_builtin_casters.py @@ -433,12 +433,13 @@ def test_const_ref_caster(): reference the casting mode used. """ x = False - assert m.takes(x) + assert m.takes(x) == 1 + assert m.takes_move(x) == 1 - assert m.takes_ptr(x) - assert m.takes_ref(x) - assert m.takes_ref_wrap(x) + assert m.takes_ptr(x) == 3 + assert m.takes_ref(x) == 2 + assert m.takes_ref_wrap(x) == 2 - assert m.takes_const_ptr(x) - assert m.takes_const_ref(x) - assert m.takes_const_ref_wrap(x) + assert m.takes_const_ptr(x) == 5 + assert m.takes_const_ref(x) == 4 + assert m.takes_const_ref_wrap(x) == 4