[smart_holder] return value policies allowed for shared_ptr #3039

jakobandersen · 2021-06-16T12:34:51Z

Description

Allow copy and move policy for smart holder casting of std::shared_ptr.

jakobandersen · 2021-06-16T13:58:06Z

@rwgk, @rhaschke, some initial code to get started.

Without -DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT I locally get a "double free or corruption" error (i guess because MyObject3a is not specified to be held by shared_ptr but is appended to the list as such anyway). This happens both after the first commit and the second commit.

With -DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT, after the first commit it gives the Invalid return_value_policy for shared_ptr. error, and after the second commit, simply allowing copy as policy in smart_holder_type_caster it seemingly works.

I'm not really sure how the whole smart holder stuff works, so any ideas of how to progress on this?
The way I added the test doesn't seem right, should it only be run in progressive mode tests?

(By the way, can you enable the workflows on the PR, it only tests in appveyor right now)

rwgk · 2021-06-16T14:32:20Z

Unfortunately I have to manually trigger the workflows each time you push something new, and I'm not getting automatic notifications. Please tag me here each time you want to run the CI (some dummy comment with @rwgk).

To make the Format workflow happy, I always blindly run pre-commit run --all-files before a git commit. See .github/CONTRIBUTING.md.

Maybe for later/general background: test_smart_ptrs.cpp,py is one of the few tests that does NOT exercise any smart_holder functionality. It would be better if you added to some test_class_sh_*.cpp,py or ideally simply make a new test_class_sh_xxx.cpp,py pair. The extra overhead is super tiny (one more line in tests/CMakeLists.txt). Then we can decide later if we want to keep the new tests separate, or fold them into something else (also super easy).

About the copy policy, when enabling it, I'd want to be certain we emulate the existing behavior exactly. We could either do this via a test that uses type_caster_base in CI-SH-AVL, and smart_holder in CI-SH-DEF, i.e. via the PYBIND11_SH_AVL macro, although I think the alternative of having one test that exercises both simultaneously would be ideal. It's actually pretty simple, combining these two tricks:

pybind11/tests/test_class_sh_basic.cpp

Line 12 in 8f6ca71

struct atyp { // Short for "any type".
pybind11/tests/test_class_sh_disowning.cpp

Line 10 in 8f6ca71

template <int SerNo> // Using int as a trick to easily generate a series of types.

The first tracks copy and move, the second adds a serial number, so that you can wrap with both py::class_<Atype<0>, std::shared_ptr<Atype<0>>> and py::classh<Atype<1>> in the same test_*.cpp, then in test_*.py you can loop over both wrapped types, for the exact same tests.

rhaschke

At first sight, I don't see a particular reason for the double-free error - unfortunately.

rhaschke · 2021-06-16T17:28:52Z

tests/test_smart_ptr.cpp

+    // this doesn't compile, should it?
+    //m.def("print_myobject3a_4", [](const std::shared_ptr<MyObject3a> *obj) { py::print((*obj)->toString()); });


No. A shared_ptr is expected to be passed by some kind of reference (or value).

No. A shared_ptr is expected to be passed by some kind of reference (or value).

I'm thinking of allowing const pointers, but definitely not non-const pointers.
Rationale: It could be useful to pass nullptr, that seems like a valid use case.

A nullptr can be represented by an empty shared_ptr. IMO we always want to pass a pointer to a held object.
The shared_ptr is just a convenient replacement for a raw pointer. Passing a pointer to a (shared) pointer doesn't make sense to me - no matter being it const or not: We cannot modify the pointer anyway.

A nullptr can be represented by an empty shared_ptr. IMO we always want to pass a pointer to a held object.
The shared_ptr is just a convenient replacement for a raw pointer. Passing a pointer to a (shared) pointer doesn't make sense to me - no matter being it const or not: We cannot modify the pointer anyway.

OK, let's keep this as-is for now. Let's just wait and see if anyone later has a strong need for it.

tests/test_smart_ptr.cpp

jakobandersen · 2021-06-18T20:05:29Z

@rwgk, thanks for the explanations. I think I made essentially the changes you suggested, and it is ready for another CI run.
The candidate fix is still the last commit (I'll squash the first two later), and before it the test_avl() fails with the casting error.

rwgk · 2021-06-18T20:11:12Z

Thanks, that looks great! Please don't worry about squashing. In my experience it's easiest and best to not mess with the history. The PR will be squashed automatically when merging.

rwgk · 2021-06-18T20:12:59Z

Please keep tagging me here for triggering the CI. 10 times a day is fine, no worries.

jakobandersen · 2021-06-19T08:56:21Z

Ping @rwgk for CI :-).

tests/test_class_sh_shared_ptr_copy.cpp

include/pybind11/detail/smart_holder_type_casters.h

rhaschke

LGTM.

tests/test_class_sh_shared_ptr_copy_move.cpp

rwgk · 2021-06-19T22:48:10Z

tests/test_class_sh_shared_ptr_copy_move.py

+
+
+def test_avl_copy():
+    m.test_avl_copy()


To be sure the copy and move behavior is as expected, something like:

l = m.test_shp_copy() assert m.get_mtxt(l[0]) == "what we expect"

You probably have to allow for differences between platforms, using regular expressions, ideally this function from @rhaschke's still-open PR:

https://github.com/pybind/pybind11/pull/2916/files#diff-e614dc3a68da8c26363419d3c06816825b1b4344e35103c5dd004f7dcf3a6f84

def check_regex(expected, actual): result = re.match(expected + "$", actual) if result is None: pytest.fail("expected: '{}' != actual: '{}'".format(expected, actual))

For this you will also need the get_mtxt helper from test_class_sh_basic.cpp.

Good idea,. Though, implemented as exact string compare, as in this case there should be no modification of the Foo objects after construction. The copy/move constructors are now deleted, just to ensure that.

I have to think about this one. We don't want to leave any holes and want to know exactly what happens in all situations incl. different platforms. Maybe we need multiple Foo types with different combinations of (copy, move) x (ctor, assignment-op)? Is some or all of that obviously non-sensical? But let's get the CI green first with what you have right now.

Is there anywhere where the pybind code is conditional on the class being moveable/copyable?
It may be good to add a move-only Foo and copyable Foo to test that indeed the object is not modified in any way, and it's only pointer copying.

rwgk · 2021-06-20T13:42:00Z

tests/test_class_sh_shared_ptr_copy_move.cpp

+    Foo(Foo &&other) = delete;
+    std::string get_text() const {
+        std::string res = "Foo";
+        if (SerNo == 0)


The appveyor build is (and probably all our 30+ MSVC builds would be) failing:

C:\projects\pybind11\tests\test_class_sh_shared_ptr_copy_move.cpp(20): warning C4127: conditional expression is constant [C:\projects\pybind11\tests\pybind11_tests.vcxproj] C:\projects\pybind11\tests\test_class_sh_shared_ptr_copy_move.cpp(22): warning C4127: conditional expression is constant [C:\projects\pybind11\tests\pybind11_tests.vcxproj]

My recommendation is to keep the code here as simple as possible (so it's easier to reason about) and to use this approach here:

pybind11/tests/test_class_sh_basic.cpp

Line 96 in 29fafcc

py::classh<atyp>(m, "atyp").def(py::init<>()).def(py::init([](const std::string &mtxt) {

Then set the initial mtxt from Python.
BTW in some more recent code I called it history, if you like that better.

Warning fixed and member renamed to history.

rwgk · 2021-06-20T13:48:42Z

tests/test_class_sh_shared_ptr_copy_move.py

+
+
+def test_avl_copy():
+    m.test_avl_copy()


I have to think about this one. We don't want to leave any holes and want to know exactly what happens in all situations incl. different platforms. Maybe we need multiple Foo types with different combinations of (copy, move) x (ctor, assignment-op)? Is some or all of that obviously non-sensical? But let's get the CI green first with what you have right now.

include/pybind11/detail/smart_holder_type_casters.h

jakobandersen · 2021-06-24T10:17:44Z

Ping @rwgk for CI, now that Appveyor is green.

rwgk

Is there anywhere where the pybind code is conditional on the class being moveable/copyable?

Yes, there are certainly conditionals, I think both compile-time (using type traits) and runtime (looking at refcounts). In and around cast.h. I don't know what all the details are. The behavior is generally well exercised in test_copy_move.cpp/py.

I'll trigger the CI now although the missing history tracking means we won't get much information.

tests/test_class_sh_shared_ptr_copy_move.cpp

rwgk · 2021-06-24T11:18:30Z

tests/test_class_sh_shared_ptr_copy_move.cpp

+    std::string hisotry;
+    Foo(const std::string &hisotry_) : hisotry(hisotry_) {}
+    Foo(const Foo &other) = delete;
+    Foo(Foo &&other)      = delete;


The purpose of the history member is completely lost: the move and copy assignment operators are not deleted, but also not tracked. How those behave is exactly what we want to test here. (At the moment idk what our conclusion will be when we see the results on all platforms, but it's important that we know what they are.)

Indeed, there should now be tracking of the special member functions.

rwgk · 2021-06-24T11:41:08Z

The many MSVC failures are due to some unrelated issue with downloading catch.hpp. I see it in other PRs as well. I'm trying to find out what's wrong.

tests/test_class_sh_shared_ptr_copy_move.cpp

rwgk · 2021-06-24T11:51:51Z

Another class of multiple CI failures are sad old compilers that complain about implicit copy constructors. Annoyingly we have to make them explicit to keep those compilers happy. I usually simply add them in without #ifdef clutter, but a comment, e.g. in test_class_sh_inheritance.cpp (// Some compilers complain about implicitly defined versions of some of the following:). That's a little overkill but I didn't want to torture our CI too much or spend a lot of time messing with docker containers.

rwgk · 2021-06-24T13:37:01Z

Unfortunately we're currently blind to the MSVC behavior (2 x 18 builds), but everything else is working. Except for clang-tidy wanting two noexcept. See below.

Did you already clang-format the new code? (clang-format -style=file -i test_class_sh_shared_ptr_copy_move.cpp)
I'm trying to keep all smart_holder code clang-formatted, but currently that is not enforced in the CI runs.

Do you feel you could add in something like this (snippet from test_shared_ptrs.cpp):

        .def_property_readonly(                                                 
            "holder_copy",                                                      
            [](const SharedPtrRef &s) { return s.shared; },                     
            py::return_value_policy::copy)

And similar for py::return_value_policy::move.
I'm not sure what exactly makes sense. I'd probably start by emulating the existing test. What I think we need to know is the history of the returned value (copied or moved), and also that of s.shared in the snippet above. If you feel that's too involved for this PR we could merge first, although I'd feel a bit uneasy about enabling the return value policies without being sure what exactly the behavior is. If it's out there for a while it will become difficult to change, even if inconsistent with the non-smart_holder behavior, and then we may have to explain any inconsistencies forever.

/__w/pybind11/pybind11/tests/test_class_sh_shared_ptr_copy_move.cpp:19:5: error: move constructors should be marked noexcept [performance-noexcept-move-constructor,-warnings-as-errors]
    Foo(Foo &&other) : history(other.history + "_MvCtor") {}
    ^
                      noexcept 
/__w/pybind11/pybind11/tests/test_class_sh_shared_ptr_copy_move.cpp:24:10: error: move assignment operators should be marked noexcept [performance-noexcept-move-constructor,-warnings-as-errors]
    Foo &operator=(Foo &&other) {
         ^
                                 noexcept

rwgk · 2021-06-24T19:10:57Z

I just pushed a temporary patch to the smart_holder branch with a workaround for the Windows breakages. Could you please rebase your branch and force push? Then we should be able to get a full set of CI results.

jakobandersen · 2021-06-24T19:39:27Z

I just pushed a temporary patch to the smart_holder branch with a workaround for the Windows breakages. Could you please rebase your branch and force push? Then we should be able to get a full set of CI results.

Sure, done.

rwgk · 2021-06-24T19:59:42Z

I pushed trivial automatic clang-tidy and clang-format fixes directly to your fork.
(Looks like it worked. This was the first time I tried that.)

rwgk · 2021-06-24T20:25:38Z

The CI is looking great now. But the behavior test I'm most worried about (something like the .def_property_readonly(..., py::return_value_policy::copy) and move) is still missing.

jakobandersen · 2021-06-29T11:18:22Z

I pushed trivial automatic clang-tidy and clang-format fixes directly to your fork.
(Looks like it worked. This was the first time I tried that.)

Great, thanks.

The CI is looking great now. But the behavior test I'm most worried about (something like the .def_property_readonly(..., py::return_value_policy::copy) and move) is still missing.

I have pushed a few commits. Is it something along those lines you mean?
It tries retrieving an attribute of type FooShPtr and FooSmHld, defined via def_readonly, def_readwrite, and def_property_readonly, in policy combinations move, copy, and whatever the default is.

rwgk · 2021-06-29T12:57:18Z

Wow, thanks! Those macros are interesting! (apparently clang-format has a rough time with them)

It looks like the Foo's copy/move/operator= are never invoked, because they never appear in the history. See below.

In other words: When returning std::shared_ptr, the copy and move return value policies have no effect at all, compared to the default policy (I think it's automatic).

Probably not what I would have done, but if that's established behavior, and we have a test for it now to ensure the behavior is identical for std::shared_ptr as holder and smart_holder, I agree it's good to get this in.

No need for more tests IMO (e.g. move-only Foo, copy-only Foo). It would be nice to parameterize test_properties, which could probably be done with itertools.product, but maybe not worth the trouble (it only adds value if some but not all tests are failing).

I see the CI is green! I'll merge this now. If we want to do anything else we can just create new PRs.

$ grep -e _CpCtor -e _MvCtor -e _OpEqLv -e _OpEqRv tests/test_class_sh_shared_ptr_copy_move.*
tests/test_class_sh_shared_ptr_copy_move.cpp:    Foo(const Foo &other) : history(other.history + "_CpCtor") {}
tests/test_class_sh_shared_ptr_copy_move.cpp:    Foo(Foo &&other) noexcept : history(other.history + "_MvCtor") {}
tests/test_class_sh_shared_ptr_copy_move.cpp:        history = other.history + "_OpEqLv";
tests/test_class_sh_shared_ptr_copy_move.cpp:        history = other.history + "_OpEqRv";

rwgk · 2021-06-29T13:10:17Z

FYI: The Rebase merge was an accident (my intention was to Squash). I didn't realize the GUI keeps the previous choice. But it's OK, I'll pay more attention next time.

jakobandersen · 2021-06-29T13:50:30Z

Thanks!

I tried a bit to make clang format not mangle it too much, but perhaps disabling it around that code may be better.

The semantics of the attributes/properties with the default policy also matches what happens if the code is directly transcribed to Boost.Python.
I guess a similar property test with unique_ptr could be interesting as well.

rwgk · 2021-06-29T13:57:01Z

Thanks!

I tried a bit to make clang format not mangle it too much, but perhaps disabling it around that code may be better.

Yeah, I was thinking the same.

The semantics of the attributes/properties with the default policy also matches what happens if the code is directly transcribed to Boost.Python.

Interesting to know. One more reason to go with the flow.

I guess a similar property test with unique_ptr could be interesting as well.

Again same thought, but I didn't want to bother you too much with requests :-)

jakobandersen · 2021-06-29T14:04:49Z

The semantics of the attributes/properties with the default policy also matches what happens if the code is directly transcribed to Boost.Python.

Interesting to know. One more reason to go with the flow.

Indeed, there shouldn't be nasty surprises. Though, they do have a type of return value policy as well, but I'm not sure if they semantically map cleanly. But it may be worth checking out if there are strange edge cases. See e.g., https://wiki.python.org/moin/boost.python/CallPolicy

I guess a similar property test with unique_ptr could be interesting as well.

Again same thought, but I didn't want to bother you too much with requests :-)

No problem. I might make a PR for it at some point, now that I know the technical setup.

Btw. I couldn't immediately find it, but what is the status of the smart holder branch with respect to a merge into master?

rwgk · 2021-06-29T14:13:09Z

Btw. I couldn't immediately find it, but what is the status of the smart holder branch with respect to a merge into master?

Earlier this year I proposed a "road map" that @henryiii used as a starting point for https://github.com/pybind/pybind11/wiki/Roadmap.

We're still figuring out how to get this work done.

In the meantime I'm striving to merge everything from master asap, keeping the smart_holder behavior in line with master as much as possible.

@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>

jakobandersen mentioned this pull request Jun 16, 2021

[smart_holder] RuntimeError: Invalid return_value_policy for shared_ptr #3011

Closed

jakobandersen force-pushed the smart_holder_shared_ptr branch 2 times, most recently from 325b522 to 79e625d Compare June 16, 2021 13:45

rhaschke reviewed Jun 16, 2021

View reviewed changes

jakobandersen force-pushed the smart_holder_shared_ptr branch from 79e625d to e950b28 Compare June 18, 2021 19:51

jakobandersen requested a review from henryiii as a code owner June 18, 2021 19:51

jakobandersen force-pushed the smart_holder_shared_ptr branch from e950b28 to f653b62 Compare June 19, 2021 08:49

rhaschke reviewed Jun 19, 2021

View reviewed changes

tests/test_class_sh_shared_ptr_copy.cpp Outdated Show resolved Hide resolved

include/pybind11/detail/smart_holder_type_casters.h Outdated Show resolved Hide resolved

rhaschke approved these changes Jun 19, 2021

View reviewed changes

jakobandersen changed the title ~~[WIP, smart_holder] return value policies allowed for shared_ptr~~ [smart_holder] return value policies allowed for shared_ptr Jun 19, 2021

rwgk reviewed Jun 19, 2021

View reviewed changes

rwgk reviewed Jun 20, 2021

View reviewed changes

include/pybind11/detail/smart_holder_type_casters.h Show resolved Hide resolved

rwgk reviewed Jun 24, 2021

View reviewed changes

tests/test_class_sh_shared_ptr_copy_move.cpp Show resolved Hide resolved

jakobandersen added 4 commits June 24, 2021 21:14

Initial test

effa01c

SH, update shared_ptr copy tests

76610ae

SH, test update

20a9495

Allow copy policy in smart holder caster for shared_ptr

4fc34e2

jakobandersen added 7 commits June 24, 2021 21:14

SH, improve error message from shared_ptr cast policy check

d9691f0

SH, shared_ptr copy/move, update after review

49878d2

SH, shared_ptr copy/move, make MSVC happy

1703361

SH, shared_ptr copy/move, rename to 'history'

1a58654

SH, fix typo

a6fa6d1

SH, fix use of PYBIND11_TYPE_CASTER_BASE_HOLDER

cb7cee8

SH, history tracking

53a04e8

jakobandersen force-pushed the smart_holder_shared_ptr branch from 92a1e1f to 53a04e8 Compare June 24, 2021 19:15

rwgk added 2 commits June 24, 2021 12:53

Automatic clang-tidy fixes.

19ca146

Automatic clang-format.

505179b

jakobandersen added 2 commits June 29, 2021 12:35

SH, attribute and property tests

a84c18e

SM, remove type annotations in test

662e515

rwgk merged commit b9241c0 into pybind:smart_holder Jun 29, 2021

github-actions bot added the needs changelog Possibly needs a changelog entry label Jun 29, 2021

rwgk removed the needs changelog Possibly needs a changelog entry label Jun 29, 2021

rwgk added a commit that referenced this pull request Jun 29, 2021

Pure clang-format fixes (minor accident in PR #3039).

bcad852

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3039

Closed

rwgk mentioned this pull request Mar 5, 2025

squash-merge smart_holder branch into master #5542

Merged

		// this doesn't compile, should it?
		//m.def("print_myobject3a_4", [](const std::shared_ptr<MyObject3a> obj) { py::print((obj)->toString()); });

[smart_holder] return value policies allowed for shared_ptr #3039

[smart_holder] return value policies allowed for shared_ptr #3039

Conversation

jakobandersen commented Jun 16, 2021 • edited by rwgk Loading

Description

jakobandersen commented Jun 16, 2021

rwgk commented Jun 16, 2021

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobandersen commented Jun 18, 2021

rwgk commented Jun 18, 2021

rwgk commented Jun 18, 2021

jakobandersen commented Jun 19, 2021

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobandersen commented Jun 24, 2021

rwgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk commented Jun 24, 2021

rwgk commented Jun 24, 2021

rwgk commented Jun 24, 2021 • edited Loading

rwgk commented Jun 24, 2021

jakobandersen commented Jun 24, 2021

rwgk commented Jun 24, 2021

rwgk commented Jun 24, 2021

jakobandersen commented Jun 29, 2021 • edited Loading

rwgk commented Jun 29, 2021

rwgk commented Jun 29, 2021

jakobandersen commented Jun 29, 2021

rwgk commented Jun 29, 2021

jakobandersen commented Jun 29, 2021

rwgk commented Jun 29, 2021

jakobandersen commented Jun 16, 2021 •

edited by rwgk

Loading

rwgk commented Jun 24, 2021 •

edited

Loading

jakobandersen commented Jun 29, 2021 •

edited

Loading