Skip to content

Commit

Permalink
Introduce return_value_policy_pack (will need follow-on adjustments) (
Browse files Browse the repository at this point in the history
#30011)

* [smart_holder] Implement `try_as_void_ptr_capsule` as a free function  (#4539)

* Move try_as_void_ptr_capsule out from modified_type_caster_generic_load_impl.

* Try fixing clangtidy

* Introduce return_value_policy_pack

Currently only for string_caster, tuple_caster, map_caster

* Add return_value_policy_pack::override_policy() helpers.

* PYBIND11_TYPE_CASTER_RVPP return_value_policy_pack

* Systematically use return_value_policy_pack in stl.h

* clang-tidy auto fix

* Fix MSVC warning C4458: declaration of 'policy' hides class member

* Fix oversight (when renaming return_value_policy_opts to return_value_policy_pack).

* Cover all changed casters in stl.h

* WIP callbacks

* Explicitly specify return_value_policy::automatic_reference in functional.h

* WIP proof of concept manipulating the return value policy for callbacks.

* Introduce from_python_policies as generalization of load bool convert

* Change `std::vector<bool> args_convert;` to `std::vector<from_python_policies> args_policies;`

* clang-tidy auto fix

* WIP: tests pass

* Replace `argument_record.convert`,`none` with `from_python_policies`

Core diff:

```
diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h
-    bool Zonvert : 1;  ///< True if the argument is allowed to convert when loading
-    bool Mone : 1;     ///< True if None is allowed when loading
+    from_python_policies policies;
```

* functional.h using fpp.rvpp (instead of fpp.convert) but arg is non-constexpr

-        value = func_wrapper(func_handle(std::move(func)), fpp.convert);
+        value = func_wrapper(func_handle(std::move(func)), fpp.rvpp);

Passes:

scons -j 24 selected_test_cpp=test_exceptions.cpp,test_return_value_policy_pack.cpp && /usr/bin/python3 $HOME/clone/pybind11_scons/run_tests.py ../pybind11 test_return_value_policy_pack -s -vv

But many other tests broken because arg is non-constexpr and therefore this had to be commented out in cast.h:

constexpr arg operator"" _a(const char *name, size_t) { return arg(name); }

* Split out `detail::arg_literal` to make ALL tests work again.

* clang-format auto fixes

* Quick workaround for MSVC private/friend issue.

* Add test_nested_callbacks_rtn_string

* WIP: Add collect_arguments_rvpp() functions (unused).

* Use collect_arguments_rvpp() from object_api<Derived>::operator()

* clang-format auto fixes

* Make test_return_value_policy_pack.cpp compatible with C++11 (skip optional, variant). Also fix oversight: missing Python-side test for variant.

* Add test_return_value_policy_pack to tests/CMakeLists.txt and clang-tidy auto fix.

* Resolve `-Wmissing-braces` emitted by old clang versions (3.6, 3.7, 3.9, 5):

```
test_return_value_policy_pack.cpp:51:70: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
```

* Resolve "pointless comparison of unsigned integer with zero" warnings emitted by CUDA and ICC:

CUDA 11.7 Ubuntu 22.04:
```
cast.h(1318): error #186-D: pointless comparison of unsigned integer with zero
```

ICC latest x64 (Intel 2021.8.0.20221119):
```
cast.h(1318): error #186: pointless comparison of unsigned integer with zero
```

* object_api<Derived>::call_with_policies()

* Use object_api<Derived>::call_with_policies() in functional.h

* test_call_callback_pass_pair_string

* Systematically exercise nested callbacks to level 4.

* make_tuple -> make_tuple_rvpp

* simple_collector -> simple_collector_rvpp

* unpacking_collector -> unpacking_collector_rvpp

* PYBIND11_TYPE_CASTER_IMPL

* Universally pass return_value_policy_pack via const &

* Rename arg_literal to arg_base

* Fix git rebase accident.

* Suppress MINGW `-Wmismatched-new-delete` (seen in mingw32 & mingw64 ci.yml logs).

```
2023-02-17T09:47:45.7114041Z D:\a\pybind11\pybind11\tests\test_factory_constructors.cpp:381:30: error: 'void operator delete(void*)' called on pointer returned from a mismatched allocation function [-Werror=mismatched-new-delete]
2023-02-17T09:47:45.7114692Z   381 |             ::operator delete(p);
2023-02-17T09:47:45.7115055Z       |             ~~~~~~~~~~~~~~~~~^~~
2023-02-17T09:47:45.7115444Z In lambda function,
2023-02-17T09:47:45.7116873Z     inlined from 'pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, int)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, int), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, int)>' at D:/a/pybind11/pybind11/include/pybind11/detail/init.h:376:33,
2023-02-17T09:47:45.7119232Z     inlined from 'Return pybind11::detail::argument_loader<Args>::call_impl(Func&&, pybind11::detail::index_sequence<Is ...>, Guard&&) && [with Return = void; Func = pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, int)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, int), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, int)>&; long long unsigned int ...Is = {0, 1, 2}; Guard = pybind11::detail::void_type; Args = {pybind11::detail::value_and_holder&, int, int}]' at D:/a/pybind11/pybind11/include/pybind11/cast.h:1563:37,
2023-02-17T09:47:45.7122014Z     inlined from 'pybind11::detail::enable_if_t<std::is_void<_Dummy>::value, pybind11::detail::void_type> pybind11::detail::argument_loader<Args>::call(Func&&) && [with Return = void; Guard = pybind11::detail::void_type; Func = pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, int)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, int), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, int)>&; Args = {pybind11::detail::value_and_holder&, int, int}]' at D:/a/pybind11/pybind11/include/pybind11/cast.h:1537:65,
2023-02-17T09:47:45.7125679Z     inlined from 'pybind11::cpp_function::initialize<pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, int)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, int), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, int)>, void, pybind11::detail::value_and_holder&, int, int, pybind11::name, pybind11::is_method, pybind11::sibling, pybind11::detail::is_new_style_constructor>(pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, int)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, int), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, int)>&&, void (*)(pybind11::detail::value_and_holder&, int, int), const pybind11::name&, const pybind11::is_method&, const pybind11::sibling&, const pybind11::detail::is_new_style_constructor&)::<lambda(pybind11::detail::function_call&)>' at D:/a/pybind11/pybind11/include/pybind11/pybind11.h:249:71,
2023-02-17T09:47:45.7130204Z     inlined from 'static pybind11::handle pybind11::cpp_function::initialize<pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, int)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, int), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, int)>, void, pybind11::detail::value_and_holder&, int, int, pybind11::name, pybind11::is_method, pybind11::sibling, pybind11::detail::is_new_style_constructor>(pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, int)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, int), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, int)>&&, void (*)(pybind11::detail::value_and_holder&, int, int), const pybind11::name&, const pybind11::is_method&, const pybind11::sibling&, const pybind11::detail::is_new_style_constructor&)::<lambda(pybind11::detail::function_call&)>::_FUN(pybind11::detail::function_call&)' at D:/a/pybind11/pybind11/include/pybind11/pybind11.h:224:21:
2023-02-17T09:47:45.7132694Z D:\a\pybind11\pybind11\tests\test_factory_constructors.cpp:399:71: note: returned from 'static void* test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc::operator new(size_t)'
2023-02-17T09:47:45.7133385Z   399 |     pyNoisyAlloc.def(py::init([](int i, int) { return new NoisyAlloc(i); }));
2023-02-17T09:47:45.7133874Z       |                                                                       ^
2023-02-17T09:47:45.7134610Z In static member function 'static void test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc::operator delete(void*, size_t)',
2023-02-17T09:47:45.7135502Z     inlined from 'test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, double)>' at D:\a\pybind11\pybind11\tests\test_factory_constructors.cpp:408:74,
2023-02-17T09:47:45.7139172Z     inlined from 'pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, double)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, double), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, double)>' at D:/a/pybind11/pybind11/include/pybind11/detail/init.h:376:33,
2023-02-17T09:47:45.7141646Z     inlined from 'Return pybind11::detail::argument_loader<Args>::call_impl(Func&&, pybind11::detail::index_sequence<Is ...>, Guard&&) && [with Return = void; Func = pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, double)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, double), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, double)>&; long long unsigned int ...Is = {0, 1, 2}; Guard = pybind11::detail::void_type; Args = {pybind11::detail::value_and_holder&, int, double}]' at D:/a/pybind11/pybind11/include/pybind11/cast.h:1563:37,
2023-02-17T09:47:45.7144252Z     inlined from 'pybind11::detail::enable_if_t<std::is_void<_Dummy>::value, pybind11::detail::void_type> pybind11::detail::argument_loader<Args>::call(Func&&) && [with Return = void; Guard = pybind11::detail::void_type; Func = pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, double)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, double), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, double)>&; Args = {pybind11::detail::value_and_holder&, int, double}]' at D:/a/pybind11/pybind11/include/pybind11/cast.h:1537:65,
2023-02-17T09:47:45.7147880Z     inlined from 'pybind11::cpp_function::initialize<pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, double)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, double), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, double)>, void, pybind11::detail::value_and_holder&, int, double, pybind11::name, pybind11::is_method, pybind11::sibling, pybind11::detail::is_new_style_constructor>(pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, double)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, double), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, double)>&&, void (*)(pybind11::detail::value_and_holder&, int, double), const pybind11::name&, const pybind11::is_method&, const pybind11::sibling&, const pybind11::detail::is_new_style_constructor&)::<lambda(pybind11::detail::function_call&)>' at D:/a/pybind11/pybind11/include/pybind11/pybind11.h:249:71,
2023-02-17T09:47:45.7358442Z     inlined from 'static pybind11::handle pybind11::cpp_function::initialize<pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, double)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, double), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, double)>, void, pybind11::detail::value_and_holder&, int, double, pybind11::name, pybind11::is_method, pybind11::sibling, pybind11::detail::is_new_style_constructor>(pybind11::detail::initimpl::factory<test_submodule_factory_constructors(pybind11::module_&)::<lambda(int, double)>, pybind11::detail::void_type (*)(), test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc*(int, double), pybind11::detail::void_type()>::execute<pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc> >(pybind11::class_<test_submodule_factory_constructors(pybind11::module_&)::NoisyAlloc>&) &&::<lambda(pybind11::detail::value_and_holder&, int, double)>&&, void (*)(pybind11::detail::value_and_holder&, int, double), const pybind11::name&, const pybind11::is_method&, const pybind11::sibling&, const pybind11::detail::is_new_style_constructor&)::<lambda(pybind11::detail::function_call&)>::_FUN(pybind11::detail::function_call&)' at D:/a/pybind11/pybind11/include/pybind11/pybind11.h:224:21:
```

* Fix copy-paste mishap (thanks @lalaland for catching this).

* ruff auto-fixes

* Undo temporary change to test_gil_scoped.py

---------

Co-authored-by: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com>
  • Loading branch information
rwgk and wangxf123456 authored Mar 16, 2023
1 parent f8128f0 commit c5bb69e
Show file tree
Hide file tree
Showing 12 changed files with 863 additions and 140 deletions.
48 changes: 34 additions & 14 deletions include/pybind11/attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,13 @@ struct argument_record {
const char *name; ///< Argument name
const char *descr; ///< Human-readable version of the argument value
handle value; ///< Associated Python object
bool convert : 1; ///< True if the argument is allowed to convert when loading
bool none : 1; ///< True if None is allowed when loading
from_python_policies policies;

argument_record(const char *name, const char *descr, handle value, bool convert, bool none)
: name(name), descr(descr), value(value), convert(convert), none(none) {}
argument_record(const char *name,
const char *descr,
handle value,
const from_python_policies &policies)
: name(name), descr(descr), value(value), policies(policies) {}
};

/// Internal data structure which holds metadata about a bound function (signature, overloads,
Expand Down Expand Up @@ -212,8 +214,8 @@ struct function_record {
/// Pointer to custom destructor for 'data' (if needed)
void (*free_data)(function_record *ptr) = nullptr;

/// Return value policy associated with this function
return_value_policy policy = return_value_policy::automatic;
/// Return value policies associated with this function
return_value_policy_pack rvpp;

/// True if name == '__init__'
bool is_constructor : 1;
Expand Down Expand Up @@ -359,7 +361,7 @@ struct type_record {

inline function_call::function_call(const function_record &f, handle p) : func(f), parent(p) {
args.reserve(f.nargs);
args_convert.reserve(f.nargs);
args_policies.reserve(f.nargs);
}

/// Tag for a new-style `__init__` defined in `detail/init.h`
Expand Down Expand Up @@ -407,7 +409,12 @@ struct process_attribute<char *> : process_attribute<const char *> {};
/// Process an attribute indicating the function's return value policy
template <>
struct process_attribute<return_value_policy> : process_attribute_default<return_value_policy> {
static void init(const return_value_policy &p, function_record *r) { r->policy = p; }
static void init(const return_value_policy &p, function_record *r) { r->rvpp.policy = p; }
};
template <>
struct process_attribute<return_value_policy_pack>
: process_attribute_default<return_value_policy_pack> {
static void init(const return_value_policy_pack &rvpp, function_record *r) { r->rvpp = rvpp; }
};

/// Process an attribute which indicates that this is an overloaded function associated with a
Expand Down Expand Up @@ -455,7 +462,8 @@ inline void check_kw_only_arg(const arg &a, function_record *r) {

inline void append_self_arg_if_needed(function_record *r) {
if (r->is_method && r->args.empty()) {
r->args.emplace_back("self", nullptr, handle(), /*convert=*/true, /*none=*/false);
r->args.emplace_back(
"self", nullptr, handle(), from_python_policies(/*convert=*/true, /*none=*/false));
}
}

Expand All @@ -464,19 +472,27 @@ template <>
struct process_attribute<arg> : process_attribute_default<arg> {
static void init(const arg &a, function_record *r) {
append_self_arg_if_needed(r);
r->args.emplace_back(a.name, nullptr, handle(), !a.flag_noconvert, a.flag_none);
r->args.emplace_back(
a.name,
nullptr,
handle(),
from_python_policies(a.m_policies.rvpp, !a.flag_noconvert, a.flag_none));

check_kw_only_arg(a, r);
}
};
template <>
struct process_attribute<detail::arg_base> : process_attribute<arg> {};

/// Process a keyword argument attribute (*with* a default value)
template <>
struct process_attribute<arg_v> : process_attribute_default<arg_v> {
static void init(const arg_v &a, function_record *r) {
if (r->is_method && r->args.empty()) {
r->args.emplace_back(
"self", /*descr=*/nullptr, /*parent=*/handle(), /*convert=*/true, /*none=*/false);
r->args.emplace_back("self",
/*descr=*/nullptr,
/*parent=*/handle(),
from_python_policies(/*convert=*/true, /*none=*/false));
}

if (!a.value) {
Expand Down Expand Up @@ -505,7 +521,11 @@ struct process_attribute<arg_v> : process_attribute_default<arg_v> {
"more information.");
#endif
}
r->args.emplace_back(a.name, a.descr, a.value.inc_ref(), !a.flag_noconvert, a.flag_none);
r->args.emplace_back(
a.name,
a.descr,
a.value.inc_ref(),
from_python_policies(a.m_policies.rvpp, !a.flag_noconvert, a.flag_none));

check_kw_only_arg(a, r);
}
Expand Down Expand Up @@ -667,7 +687,7 @@ using extract_guard_t = typename exactly_one_t<is_call_guard, call_guard<>, Extr

/// Check the number of named arguments at compile time
template <typename... Extra,
size_t named = constexpr_sum(std::is_base_of<arg, Extra>::value...),
size_t named = constexpr_sum(std::is_base_of<arg_base, Extra>::value...),
size_t self = constexpr_sum(std::is_same<is_method, Extra>::value...)>
constexpr bool expected_num_args(size_t nargs, bool has_args, bool has_kwargs) {
PYBIND11_WORKAROUND_INCORRECT_MSVC_C4100(nargs, has_args, has_kwargs);
Expand Down
Loading

0 comments on commit c5bb69e

Please sign in to comment.