[smart_holder] Auto select return value policy for clif_automatic #4381

wangxf123456 · 2022-12-02T22:13:21Z

Description

PyCLIF automatically chooses lifetime management of return values based on the properties of them: https://github.com/google/clif/tree/main/clif/python#pointers-references-and-object-ownership. This PR should make the return value policy selection consistent with PyCLIF.

Note that for const raw pointers, PyCLIF is also making a copy: https://github.com/google/clif/blob/main/clif/python/clif_types.py#L124. This is not mentioned in the link above.

Skylion007 · 2022-12-02T22:15:57Z

tests/test_return_value_policy_override.cpp

+
+struct nocopy {
+    std::string mtxt;
+    nocopy(const std::string &mtxt_) : mtxt(mtxt_) {}


Don't you still need to delete the copy ctor?

Thanks for pointing this out. I forgot this. I deleted the copy ctor.

Skylion007 · 2022-12-02T23:20:53Z

tests/test_return_value_policy_override.cpp

@@ -51,9 +53,13 @@ const obj &return_const_reference() {
    return value;
 }

-std::shared_ptr<obj> return_shared_pointer() { return std::make_shared<obj>("shared_pointer"); }
+std::shared_ptr<obj> return_shared_pointer() {
+    return std::shared_ptr<obj>(new obj("shared_pointer"));


Should be std::make_shared, right?

Initially I used std::make_shared and std::make_unqiue, but then I see errors note: ‘std::make_unique’ is only available from C++14 onwards, so I modified both of them. I should be able to use std::make_shared with C++11. I will update the PR.

rwgk · 2022-12-03T07:47:21Z

tests/test_return_value_policy_override.cpp

 #include "pybind11_tests.h"

 namespace test_return_value_policy_override {

 struct some_type {};

+struct obj {


obj is usually used for instances, it's surprising as a type name.

The combinations of copyable/movable keep coming up, it would be good have some systematic naming that we could use throughout. What do you think about

struct type_cp1_mv1 struct type_cp1_mv0 struct type_cp0_mv1 struct type_cp0_mv0

?

With a comment like:

// cp = copyable, mv = movable, 1 = yes, 0 = no

Question that's maybe beyond this PR: do we have (or would it be safest) to add tests for the other two combinations?

IIUC here you have only type_cp1_mv1 and type_cp0_mv1, is that correct?

Thanks for your suggestions. I updated the PR with more tests.

…atic (pybind#4381)" This reverts commit c1f14f0. Conflicts resolved in: include/pybind11/detail/smart_holder_type_casters.h tests/test_return_value_policy_override.py

…atic (#4381)" This reverts commit c1f14f0. Conflicts resolved in: include/pybind11/detail/smart_holder_type_casters.h tests/test_return_value_policy_override.py

@rhaschke

…major and/or influential contributors to smart_holder branch * pybind#2904 by @rhaschke was merged on Mar 16, 2021 * pybind#3012 by @rhaschke was merged on May 28, 2021 * pybind#3039 by @jakobandersen was merged on Jun 29, 2021 * pybind#3048 by @Skylion007 was merged on Jun 18, 2021 * pybind#3588 by @virtuald was merged on Jan 3, 2022 * pybind#3633 by @wangxf123456 was merged on Jan 25, 2022 * pybind#3635 by @virtuald was merged on Jan 26, 2022 * pybind#3645 by @wangxf123456 was merged on Jan 25, 2022 * pybind#3796 by @wangxf123456 was merged on Mar 10, 2022 * pybind#3807 by @wangxf123456 was merged on Mar 18, 2022 * pybind#3838 by @wangxf123456 was merged on Apr 15, 2022 * pybind#3929 by @tomba was merged on May 7, 2022 * pybind#4031 by @wangxf123456 was merged on Jun 27, 2022 * pybind#4343 by @wangxf123456 was merged on Nov 18, 2022 * pybind#4381 by @wangxf123456 was merged on Dec 5, 2022 * pybind#4539 by @wangxf123456 was merged on Feb 28, 2023 * pybind#4609 by @wangxf123456 was merged on Apr 6, 2023 * pybind#4775 by @wangxf123456 was merged on Aug 3, 2023 * pybind#4921 by @iwanders was merged on Nov 7, 2023 * pybind#4924 by @iwanders was merged on Nov 6, 2023 * pybind#5401 by @msimacek was merged on Oct 8, 2024 Co-authored-by: Aaron Gokaslan <aaronGokaslan@gmail.com> Co-authored-by: Dustin Spicuzza <dustin@virtualroadside.com> Co-authored-by: Ivor Wanders <iwanders@users.noreply.github.com> Co-authored-by: Jakob Lykke Andersen <Jakob@caput.dk> Co-authored-by: Michael Šimáček <michael.simacek@oracle.com> Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com> Co-authored-by: Tomi Valkeinen <tomi.valkeinen@iki.fi> Co-authored-by: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com>

@virtuald

* Pure `git merge --squash smart_holder` (no manual interventions). * Remove ubench/ directory. * Remove include/pybind11/smart_holder.h * [ci skip] smart_ptrs.rst updates [WIP/unfinished] * [ci skip] smart_ptrs.rst updates continued; also updating classes.rst, advanced/classes.rst * Remove README_smart_holder.rst * Restore original README.rst from master * [ci skip] Minimal change to README.rst, to leave a hint that this is pybind11v3 * [ci skip] Work in ChatGPT suggestions. * Change macro name to PYBIND11_RUN_TESTING_WITH_SMART_HOLDER_AS_DEFAULT_BUT_NEVER_USE_IN_PRODUCTION_PLEASE * Add a note pointing to the holder reinterpret_cast. * Incorporate suggestion by @virtuald: #5542 (comment) * Systematically change most py::class_ to py::classh under docs/ * Remove references to README_smart_holder.rst This should have been part of commit eb550d0. * [ci skip] Fix minor oversight (``class_`` -> ``py::class_``) noticed by chance. * [ci skip] Resolve suggestion by @virtuald #5542 (comment) * [ci skip] Apply suggestions by @timohl (thanks!) * #5542 (comment) * #5542 (comment) * #5542 (comment) * Replace `classh : class_` inhertance with `using`, as suggested by @henryiii #5542 (comment) * Revert "Systematically change most py::class_ to py::classh under docs/" This reverts commit ac9d31e. * docs: focus on py::smart_holder instead of py::classh Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * Restore minor general fixes that got lost when ac9d31e was reverted. * Remove `- smart_holder` from list of branches in all .github/workflows * Extend classh note to explain whitespace noise motivation. * Suggest `py::smart_holder` for "most situations for safety" * Add back PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT This define was * introduced with #5286 * removed with #5531 It is has been in use here: * https://github.com/pybind/pybind11_protobuf/blob/f02a2b7653bc50eb5119d125842a3870db95d251/pybind11_protobuf/native_proto_caster.h#L89-L101 Currently pybind11 unit tests for the two holder caster backwards compatibility traits * `copyable_holder_caster_shared_ptr_with_smart_holder_support_enabled` * `move_only_holder_caster_unique_ptr_with_smart_holder_support_enabled` are missing. * Add py::trampoline_self_life_support to all trampoline examples under docs/. Address suggestion by @timohl: * #5542 (comment) Add to the "please think twice" note: the overhead for safety is likely in the noise. Also fix a two-fold inconsistency introduced by revert-commit 1e646c9: 1. py::trampoline_self_life_support is mentioned in a note, but is missing in the example right before. 2. The section starting with To enable safely passing a ``std::unique_ptr`` to a trampoline object between is obsolete. * Fix whitespace accident (indentation) introduced with 1e646c9 Apparently the mis-indentation was introduced when resolving merge conflicts for what became 1e646c9 * WHITESPACE CHANGES ONLY in README.rst (list of people that made significant contributions) * Add Ethan Steinberg to list of people that made significant contributions (for completeness, unrelated to smart_holder work). * [ci skip] Add to list of people that made significant contributions: major and/or influential contributors to smart_holder branch * #2904 by @rhaschke was merged on Mar 16, 2021 * #3012 by @rhaschke was merged on May 28, 2021 * #3039 by @jakobandersen was merged on Jun 29, 2021 * #3048 by @Skylion007 was merged on Jun 18, 2021 * #3588 by @virtuald was merged on Jan 3, 2022 * #3633 by @wangxf123456 was merged on Jan 25, 2022 * #3635 by @virtuald was merged on Jan 26, 2022 * #3645 by @wangxf123456 was merged on Jan 25, 2022 * #3796 by @wangxf123456 was merged on Mar 10, 2022 * #3807 by @wangxf123456 was merged on Mar 18, 2022 * #3838 by @wangxf123456 was merged on Apr 15, 2022 * #3929 by @tomba was merged on May 7, 2022 * #4031 by @wangxf123456 was merged on Jun 27, 2022 * #4343 by @wangxf123456 was merged on Nov 18, 2022 * #4381 by @wangxf123456 was merged on Dec 5, 2022 * #4539 by @wangxf123456 was merged on Feb 28, 2023 * #4609 by @wangxf123456 was merged on Apr 6, 2023 * #4775 by @wangxf123456 was merged on Aug 3, 2023 * #4921 by @iwanders was merged on Nov 7, 2023 * #4924 by @iwanders was merged on Nov 6, 2023 * #5401 by @msimacek was merged on Oct 8, 2024 Co-authored-by: Aaron Gokaslan <aaronGokaslan@gmail.com> Co-authored-by: Dustin Spicuzza <dustin@virtualroadside.com> Co-authored-by: Ivor Wanders <iwanders@users.noreply.github.com> Co-authored-by: Jakob Lykke Andersen <Jakob@caput.dk> Co-authored-by: Michael Šimáček <michael.simacek@oracle.com> Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com> Co-authored-by: Tomi Valkeinen <tomi.valkeinen@iki.fi> Co-authored-by: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> --------- Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> Co-authored-by: Henry Schreiner <henryschreineriii@gmail.com> Co-authored-by: Aaron Gokaslan <aaronGokaslan@gmail.com> Co-authored-by: Dustin Spicuzza <dustin@virtualroadside.com> Co-authored-by: Ivor Wanders <iwanders@users.noreply.github.com> Co-authored-by: Jakob Lykke Andersen <Jakob@caput.dk> Co-authored-by: Michael Šimáček <michael.simacek@oracle.com> Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com> Co-authored-by: Tomi Valkeinen <tomi.valkeinen@iki.fi> Co-authored-by: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com>

Auto select return value policy for clif_automatic

5b7d08e

Skylion007 reviewed Dec 2, 2022

View reviewed changes

Try fixing test failures

d1b36c9

Skylion007 reviewed Dec 2, 2022

View reviewed changes

rwgk reviewed Dec 3, 2022

View reviewed changes

wangxf123456 added 6 commits December 5, 2022 11:50

Merge branch 'smart_holder' into apply-clif-automatic

b7f2e9f

Add more tests.

5b7bd24

remove comments

8f992dd

Fix test failures

853c556

Fix test failures

74a9e7a

Fix test failure for windows platform

5f310ab

wangxf123456 changed the title ~~Auto select return value policy for clif_automatic~~ [smart_holder] Auto select return value policy for clif_automatic Dec 5, 2022

Fix clangtidy

2e3c2b7

wangxf123456 marked this pull request as ready for review December 5, 2022 22:31

rwgk approved these changes Dec 5, 2022

View reviewed changes

rwgk merged commit c1f14f0 into pybind:smart_holder Dec 5, 2022

github-actions bot added the needs changelog Possibly needs a changelog entry label Dec 5, 2022

wangxf123456 deleted the apply-clif-automatic branch December 6, 2022 19:30

henryiii removed the needs changelog Possibly needs a changelog entry label Dec 19, 2022

rwgk mentioned this pull request Feb 11, 2023

FWD pybind11 google/pybind11clif#4381

Closed

rwgk mentioned this pull request Mar 5, 2025

squash-merge smart_holder branch into master #5542

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[smart_holder] Auto select return value policy for clif_automatic #4381

[smart_holder] Auto select return value policy for clif_automatic #4381

wangxf123456 commented Dec 2, 2022 •

edited

Loading

Skylion007 Dec 2, 2022

wangxf123456 Dec 2, 2022

Skylion007 Dec 2, 2022

wangxf123456 Dec 5, 2022

rwgk Dec 3, 2022

wangxf123456 Dec 5, 2022

[smart_holder] Auto select return value policy for clif_automatic #4381

[smart_holder] Auto select return value policy for clif_automatic #4381

Conversation

wangxf123456 commented Dec 2, 2022 • edited Loading

Description

Skylion007 Dec 2, 2022

Choose a reason for hiding this comment

wangxf123456 Dec 2, 2022

Choose a reason for hiding this comment

Skylion007 Dec 2, 2022

Choose a reason for hiding this comment

wangxf123456 Dec 5, 2022

Choose a reason for hiding this comment

rwgk Dec 3, 2022

Choose a reason for hiding this comment

wangxf123456 Dec 5, 2022

Choose a reason for hiding this comment

wangxf123456 commented Dec 2, 2022 •

edited

Loading