Fixes issue #3801 #3807

wangxf123456 · 2022-03-16T19:22:27Z

Description

Fixes #3801 by only calling try_as_void_ptr_capsule when the Python object is not wrapped with pybind11.

Also modified the test cases to make them work with the new implementation.

for more information, see https://pre-commit.ci

rwgk · 2022-03-16T19:45:14Z

include/pybind11/detail/smart_holder_type_casters.h

@@ -292,7 +292,8 @@ class modified_type_caster_generic_load_impl {
            loaded_v_h = value_and_holder();
            return true;
        }
-        if (convert && cpptype && try_as_void_ptr_capsule(src)) {
+        const auto &bases = all_type_info(srctype);
+        if (convert && cpptype && bases.empty() && try_as_void_ptr_capsule(src)) {


The (possibly expensive) all_type_info call is needed only if convert && cpptype is true:

if (convert && cpptype) { const auto &bases = all_type_info(srctype); if (bases.empty() && try_as_void_ptr_capsule(src)) { return true; } }

But even that is still doing too much work.
After everything is working (CI is green), but before marking this PR as ready for review, I'd look into something like has_type_info(src_type) which returns immediately when it find the first one.
Not sure if that actually makes sense, but I'd definitely look to find out.
Another idea: have a const pointer for bases right under the Case 2 comment, if set already inside Case 2, reuse here.
But that one may be a completely inconsequential optimization, although also an easy one, not sure if it's worth it or not.

There is get_type_info: https://github.com/pybind/pybind11/blob/master/include/pybind11/detail/type_caster_base.h#L184. But all_type_info is called inside. I did not find anything else that is related to this right now. But I will spend more time on this direction.

For all_type_info, looks like it will cache the result: https://github.com/pybind/pybind11/blob/master/include/pybind11/detail/type_caster_base.h#L167. So I believe if it reached Case 2, the cached result will be returned.

rwgk · 2022-03-16T19:53:05Z

tests/test_class_sh_void_ptr_capsule.py

@@ -3,26 +3,79 @@
 from pybind11_tests import class_sh_void_ptr_capsule as m


+class Valid:
+
+    capsule_generated = False


This looks dangerous.

def __init__(self): self.capsule_generated = False

maybe?
Even if the test only calls it once at the moment, I'd add that __init__. Otherwise someone could trip over this when working or experimenting with this test later.

for more information, see https://pre-commit.ci

rwgk · 2022-03-18T00:46:57Z

tests/test_class_sh_void_ptr_capsule.cpp

-    int get() const override { return 103; }
+// Create a void pointer capsule for test. The encapsulated object does not
+// matter for this test case.
+PyObject *create_test_void_ptr_capsule() { return PyCapsule_New((void *) 12345, "int", nullptr); }


While this is evidently fine on all platforms today, I'm afraid casting an arbitrary integer to a pointer could trigger some sanitizer error in the future. Ideas:

// Deemed safe not to incref. None surely outlives this capsule. static_cast<void *>(Py_None)

or

static int just_to_have_something_to_point_to = 0; static_cast<void *>(&just_to_have_something_to_point_to)

rwgk · 2022-03-18T00:49:38Z

tests/test_class_sh_void_ptr_capsule.py

+    def as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned(  # noqa: N802
+        self,
+    ):
+        return


pass

or

return None

would seem either most idiomatic or most explicit.

rwgk · 2022-03-18T15:51:51Z

Google global testing with this PR passed (2022-03-18 05:00 batch, test token OCL:435561515:BASE:435643811:1647617193745:53ea324).

for more information, see https://pre-commit.ci

… use case: `Base` not specified in `classh<Derived>` statement, but passing Derived as Base works anyway.

* Alternative approach to #3807 that supports an important PyCLIF use case: `Base` not specified in `classh<Derived>` statement, but passing Derived as Base works anyway. * NOtest_multiple_inheritance_getattr in test_class_sh_void_ptr_capsule.py (quick experiment) * Revert "NOtest_multiple_inheritance_getattr in test_class_sh_void_ptr_capsule.py (quick experiment)" This reverts commit e8f0749. * Special handling of pybind11 objects to side-step try_as_void_ptr_capsule_get_pointer __getattr__ issues. * Inspect `internals.registered_types_py` in `type_is_pybind11_class_()` * Remove debug code in tests subdir. * Clean up the modified `try_as_void_ptr_capsule_get_pointer()` implementation and new helper functions.

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

Xiaofei Wang and others added 6 commits March 16, 2022 12:18

Fixes issue

10cc0e2

[pre-commit.ci] auto fixes from pre-commit.com hooks

d107a06

for more information, see https://pre-commit.ci

Fix lint error

50bd80a

[pre-commit.ci] auto fixes from pre-commit.com hooks

26aa3a7

for more information, see https://pre-commit.ci

Fix flake8

febb4f2

[pre-commit.ci] auto fixes from pre-commit.com hooks

ca68831

for more information, see https://pre-commit.ci

rwgk reviewed Mar 16, 2022

View reviewed changes

Xiaofei Wang and others added 9 commits March 16, 2022 13:11

Fix test

ff6f41e

[pre-commit.ci] auto fixes from pre-commit.com hooks

6157d79

for more information, see https://pre-commit.ci

Fix clang tidy

3a5baa2

[pre-commit.ci] auto fixes from pre-commit.com hooks

c714d58

for more information, see https://pre-commit.ci

Fix again

cf51ccd

[pre-commit.ci] auto fixes from pre-commit.com hooks

749d3cc

for more information, see https://pre-commit.ci

Fix test

79cedf9

Add comments

d8daf83

Try fix Valgrind

84fb7b4

wangxf123456 marked this pull request as ready for review March 18, 2022 00:16

rwgk approved these changes Mar 18, 2022

View reviewed changes

wangxf123456 and others added 2 commits March 18, 2022 10:21

Resolve comments

06e5658

[pre-commit.ci] auto fixes from pre-commit.com hooks

099d1f0

for more information, see https://pre-commit.ci

rwgk merged commit 7405976 into pybind:smart_holder Mar 18, 2022

github-actions bot added the needs changelog Possibly needs a changelog entry label Mar 18, 2022

rwgk removed the needs changelog Possibly needs a changelog entry label Mar 18, 2022

rwgk mentioned this pull request Mar 18, 2022

[smart_holder] git merge master #3813

Merged

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3807

Closed

rwgk added a commit to rwgk/pybind11 that referenced this pull request Apr 8, 2023

Alternative approach to pybind#3807 that supports an important PyCLIF…

f27bd9a

… use case: `Base` not specified in `classh<Derived>` statement, but passing Derived as Base works anyway.

rwgk mentioned this pull request Mar 5, 2025

feat: squash-merge smart_holder branch into master #5542

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes issue #3801 #3807

Fixes issue #3801 #3807

wangxf123456 commented Mar 16, 2022

rwgk Mar 16, 2022

wangxf123456 Mar 16, 2022

rwgk Mar 16, 2022

rwgk Mar 18, 2022

rwgk Mar 18, 2022

rwgk commented Mar 18, 2022

Fixes issue #3801 #3807

Fixes issue #3801 #3807

Conversation

wangxf123456 commented Mar 16, 2022

Description

rwgk Mar 16, 2022

Choose a reason for hiding this comment

wangxf123456 Mar 16, 2022

Choose a reason for hiding this comment

rwgk Mar 16, 2022

Choose a reason for hiding this comment

rwgk Mar 18, 2022

Choose a reason for hiding this comment

rwgk Mar 18, 2022

Choose a reason for hiding this comment

rwgk commented Mar 18, 2022