Skip to content
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.

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

[smart_holder] Remove obsolete detail::type_info::default_holder member. #5541

Merged
merged 23 commits into from
Feb 22, 2025

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Feb 22, 2025

Description

The core change under this PR is (include/pybind11/detail/internals.h):

@@ -234,6 +234,7 @@ struct type_info {
     buffer_info *(*get_buffer)(PyObject *, void *) = nullptr;
     void *get_buffer_data = nullptr;
     void *(*module_local_load)(PyObject *, const type_info *) = nullptr;
+    holder_enum_t holder_enum_v = holder_enum_t::undefined;
     /* A simple type never occurs as a (direct or indirect) parent
      * of a class that makes use of multiple inheritance.
      * A type can be simple even if it has non-simple ancestors as long as it has no descendants.
@@ -241,13 +242,8 @@ struct type_info {
     bool simple_type : 1;
     /* True if there is no multiple inheritance in this type's inheritance tree */
     bool simple_ancestors : 1;
-    /* for base vs derived holder_type checks */
-    // SMART_HOLDER_BAKEIN_FOLLOW_ON: Remove default_holder member here and
-    // produce better error messages in the places where it is currently used.
-    bool default_holder : 1;
     /* true if this is a type registered with py::module_local */
     bool module_local : 1;
-    holder_enum_t holder_enum_v = holder_enum_t::undefined;
 };

This PR

  • only resolves the "Remove default_holder member" part (see above), but
  • does not make changes to "produce better error messages".

Rationale:

  • Removing the default_holder member requires a PYBIND11_INTERNALS_VERSION bump. To minimize bumps on master, this is done before merging smart_holder into master.
  • The changes to produce better error message are assumed to be significantly larger than this conservative PR, and they will change the pybind11 behavior. These changes are best done with reviews on the master branch (before the pybind11 v3.0.0 release).

// SMART_HOLDER_BAKEIN_FOLLOW_ON: Refine holder compatibility checks. comments are added here, so that it will be easy to pin-point where we want to produce better error messages.

Suggested changelog entry:

rwgk added 23 commits February 17, 2025 16:24
Example error message (🐍 3.11 • ubuntu-latest • x64, GNU 13.3.0):

```
include/pybind11/detail/value_and_holder.h:56:52: error: ‘uint8_t’ is not a member of ‘std’; did you mean ‘wint_t’?
   56 |             inst->nonsimple.status[index] &= (std::uint8_t) ~instance::status_holder_constructed;
      |                                                    ^~~~~~~
```
…nfo->default_holder. BUILDS BUT 2 TESTS ARE FAILING.
…ique_ptr`

Intentionally not changing error messages, because this would result in a significantly bigger change.
…a follow-on PR that actually changes the internals.
@rwgk
Copy link
Collaborator Author

rwgk commented Feb 22, 2025

@rhaschke for visibility. — I hope this will bring an end to the smart_holder / PYBIND11_INTERNALS_VERSION confusion.

@rwgk rwgk marked this pull request as ready for review February 22, 2025 19:12
@rwgk rwgk merged commit aed215c into pybind:smart_holder Feb 22, 2025
80 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Feb 22, 2025
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Feb 22, 2025
@rwgk rwgk deleted the remove_default_holder_member branch February 22, 2025 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant