From 50361b12be83fdfdc89a424169ad5f0a971d22bb Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 5 Aug 2024 20:29:21 -0700 Subject: [PATCH 1/2] Equivalent of PR #4597, but based on pybind11k main @ ab472bbce81de4e0ae41d861a5c46903968f7b27 The pybind11k `return_value_policy_pack` and `function_record` changes were backed out. The net diffs to PR #4597 are merely: ```diff +#define PYBIND11_HAS_TYPE_CASTER_STD_FUNCTION_SPECIALIZATIONS ``` ```diff - explicit func_wrapper_base(func_handle &&hf) noexcept : hfunc(hf) {} + explicit func_wrapper_base(func_handle &&hf) noexcept : hfunc(std::move(hf)) {} ``` --- include/pybind11/functional.h | 79 +++++++++++-------- tests/CMakeLists.txt | 1 + ...pe_caster_std_function_specializations.cpp | 46 +++++++++++ ...ype_caster_std_function_specializations.py | 15 ++++ 4 files changed, 107 insertions(+), 34 deletions(-) create mode 100644 tests/test_type_caster_std_function_specializations.cpp create mode 100644 tests/test_type_caster_std_function_specializations.py diff --git a/include/pybind11/functional.h b/include/pybind11/functional.h index 6856119cde..bd34710cba 100644 --- a/include/pybind11/functional.h +++ b/include/pybind11/functional.h @@ -9,12 +9,55 @@ #pragma once +#define PYBIND11_HAS_TYPE_CASTER_STD_FUNCTION_SPECIALIZATIONS + #include "pybind11.h" #include PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) PYBIND11_NAMESPACE_BEGIN(detail) +PYBIND11_NAMESPACE_BEGIN(type_caster_std_function_specializations) + +// ensure GIL is held during functor destruction +struct func_handle { + function f; +#if !(defined(_MSC_VER) && _MSC_VER == 1916 && defined(PYBIND11_CPP17)) + // This triggers a syntax error under very special conditions (very weird indeed). + explicit +#endif + func_handle(function &&f_) noexcept + : f(std::move(f_)) { + } + func_handle(const func_handle &f_) { operator=(f_); } + func_handle &operator=(const func_handle &f_) { + gil_scoped_acquire acq; + f = f_.f; + return *this; + } + ~func_handle() { + gil_scoped_acquire acq; + function kill_f(std::move(f)); + } +}; + +// to emulate 'move initialization capture' in C++11 +struct func_wrapper_base { + func_handle hfunc; + explicit func_wrapper_base(func_handle &&hf) noexcept : hfunc(std::move(hf)) {} +}; + +template +struct func_wrapper : func_wrapper_base { + using func_wrapper_base::func_wrapper_base; + Return operator()(Args... args) const { + gil_scoped_acquire acq; + // casts the returned object as a rvalue to the return type + return hfunc.f(std::forward(args)...).template cast(); + } +}; + +PYBIND11_NAMESPACE_END(type_caster_std_function_specializations) template struct type_caster> { @@ -77,40 +120,8 @@ struct type_caster> { // See PR #1413 for full details } - // ensure GIL is held during functor destruction - struct func_handle { - function f; -#if !(defined(_MSC_VER) && _MSC_VER == 1916 && defined(PYBIND11_CPP17)) - // This triggers a syntax error under very special conditions (very weird indeed). - explicit -#endif - func_handle(function &&f_) noexcept - : f(std::move(f_)) { - } - func_handle(const func_handle &f_) { operator=(f_); } - func_handle &operator=(const func_handle &f_) { - gil_scoped_acquire acq; - f = f_.f; - return *this; - } - ~func_handle() { - gil_scoped_acquire acq; - function kill_f(std::move(f)); - } - }; - - // to emulate 'move initialization capture' in C++11 - struct func_wrapper { - func_handle hfunc; - explicit func_wrapper(func_handle &&hf) noexcept : hfunc(std::move(hf)) {} - Return operator()(Args... args) const { - gil_scoped_acquire acq; - // casts the returned object as a rvalue to the return type - return hfunc.f(std::forward(args)...).template cast(); - } - }; - - value = func_wrapper(func_handle(std::move(func))); + value = type_caster_std_function_specializations::func_wrapper( + type_caster_std_function_specializations::func_handle(std::move(func))); return true; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 1007d6d827..64d4f8dd09 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -177,6 +177,7 @@ set(PYBIND11_TEST_FILES test_tagbased_polymorphic test_thread test_type_caster_pyobject_ptr + test_type_caster_std_function_specializations test_union test_unnamed_namespace_a test_unnamed_namespace_b diff --git a/tests/test_type_caster_std_function_specializations.cpp b/tests/test_type_caster_std_function_specializations.cpp new file mode 100644 index 0000000000..89213ddb19 --- /dev/null +++ b/tests/test_type_caster_std_function_specializations.cpp @@ -0,0 +1,46 @@ +#include +#include + +#include "pybind11_tests.h" + +namespace py = pybind11; + +namespace { + +struct SpecialReturn { + int value = 99; +}; + +} // namespace + +namespace pybind11 { +namespace detail { +namespace type_caster_std_function_specializations { + +template +struct func_wrapper : func_wrapper_base { + using func_wrapper_base::func_wrapper_base; + SpecialReturn operator()(Args... args) const { + gil_scoped_acquire acq; + SpecialReturn result; + try { + result = hfunc.f(std::forward(args)...).template cast(); + } catch (error_already_set &) { + result.value += 1; + } + result.value += 100; + return result; + } +}; + +} // namespace type_caster_std_function_specializations +} // namespace detail +} // namespace pybind11 + +TEST_SUBMODULE(type_caster_std_function_specializations, m) { + py::class_(m, "SpecialReturn") + .def(py::init<>()) + .def_readwrite("value", &SpecialReturn::value); + m.def("call_callback_with_special_return", + [](const std::function &func) { return func(); }); +} diff --git a/tests/test_type_caster_std_function_specializations.py b/tests/test_type_caster_std_function_specializations.py new file mode 100644 index 0000000000..9e45d4f59e --- /dev/null +++ b/tests/test_type_caster_std_function_specializations.py @@ -0,0 +1,15 @@ +from __future__ import annotations + +from pybind11_tests import type_caster_std_function_specializations as m + + +def test_callback_with_special_return(): + def return_special(): + return m.SpecialReturn() + + def raise_exception(): + raise ValueError("called raise_exception.") + + assert return_special().value == 99 + assert m.call_callback_with_special_return(return_special).value == 199 + assert m.call_callback_with_special_return(raise_exception).value == 200 From 369daf52d47b14af3eddc576f96d709fae390660 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 5 Aug 2024 21:32:26 -0700 Subject: [PATCH 2/2] Remove `std::move` to resolve clang-tidy error: ``` /__w/pybind11/pybind11/include/pybind11/functional.h:47:67: error: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg,-warnings-as-errors] 47 | explicit func_wrapper_base(func_handle &&hf) noexcept : hfunc(std::move(hf)) {} | ^~~~~~~~~~ ~ /__w/pybind11/pybind11/include/pybind11/functional.h:23:8: note: 'func_handle' is not move assignable/constructible ``` --- include/pybind11/functional.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/functional.h b/include/pybind11/functional.h index bd34710cba..4b3610117c 100644 --- a/include/pybind11/functional.h +++ b/include/pybind11/functional.h @@ -44,7 +44,7 @@ struct func_handle { // to emulate 'move initialization capture' in C++11 struct func_wrapper_base { func_handle hfunc; - explicit func_wrapper_base(func_handle &&hf) noexcept : hfunc(std::move(hf)) {} + explicit func_wrapper_base(func_handle &&hf) noexcept : hfunc(hf) {} }; template