Skip to content

Add static_asserts to enforce that py::smart_holder is combined with py::trampoline_self_life_support #5633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
May 1, 2025
Merged

Add static_asserts to enforce that py::smart_holder is combined with py::trampoline_self_life_support #5633

merged 6 commits into from
May 1, 2025
+56 −87

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Apr 28, 2025 •
edited
Loading

Description

Context:

In some long ChatGPT conversation related to PR #5624 rwgk wrote:

Is it helpful to add this explanation?

py::trampoline_self_life_support is a smart_holder-only feature. The py::trampoline_self_life_support code is not used if a shared_ptr is the holder.

ChatGPT said:

Yes — that clarification is extremely helpful and very much worth stating explicitly, especially because:

The name py::trampoline_self_life_support makes it sound like it's part of the general trampoline machinery.

Many readers (even experienced ones) will assume that shared_ptr-is-holder also uses it, since it seems designed for that use case.

Even in this conversation, the earlier assumption (before your clarification) was that shared_ptr holder behavior might involve py::trampoline_self_life_support.

A changelog entry is not needed because the smart_holder feature was not released yet.

rwgk added 6 commits April 28, 2025 14:32
@rwgk
Strictly enforce: trampoline must inherit from trampoline_self_life_s…
b3b3a54 
…upport when used in combination with smart_holder
@rwgk
Simplify test_class_sh_trampoline_basic.cpp,py (only one Abase is nee…
34ce2d9 
…ded now)
@rwgk
Replace obsolete sophisticated throw value_error() with a simple `a…
a502603 
…ssert()`
@rwgk
Strictly enforce: trampoline should inherit from trampoline_self_life…
25108cf 
…_support only if used in combination with smart_holder
@rwgk
Resolve clang-tidy error
3863958 
```
/__w/pybind11/pybind11/tests/test_class_sh_trampoline_basic.cpp:35:46: error: the parameter 'obj' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
   35 | int AddInCppSharedPtr(std::shared_ptr<Abase> obj, int other_val) {
      |                                              ^
      |                       const                 &
```
@rwgk
Disable new static_assert if PYBIND11_RUN_TESTING_WITH_SMART_HOLDER_A…
a5e6e01 
…S_DEFAULT_BUT_NEVER_USE_IN_PRODUCTION_PLEASE is defined.
@rwgk rwgk marked this pull request as ready for review April 29, 2025 15:09
@rwgk rwgk mentioned this pull request Apr 29, 2025
Draft
@virtuald
Copy link
Contributor

virtuald commented May 1, 2025

This seems like a good idea to me. 👍

@rwgk
Copy link
Collaborator Author

rwgk commented May 1, 2025

Thanks @virtuald! I'll merge this PR now.

Thanks also for the other review. I'll work on changes there asap. (For today my brain is fried...)

@rwgk rwgk merged commit c630e22 into pybind:master May 1, 2025
159 of 161 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label May 1, 2025
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label May 1, 2025
@rwgk rwgk deleted the static_assert_holder_vs_trampoline_self_life_support branch May 1, 2025 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rwgk @virtuald