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

Small cleanup/refactoring in support of PR #5213 #5251

Merged
merged 3 commits into from
Jul 19, 2024

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Jul 18, 2024

Description

See PR #5213 for the context from which these changes originate:


commit 5e30064:

Factor out detail/value_and_holder.h (from detail/type_caster_base.h).

This is in support of PR #5213:

  • trampoline_self_life_support.h depends on value_and_holder.h

  • type_caster_base.h depends on trampoline_self_life_support.h


commit 54fd559:

  • Fix a minor and inconsequential inconsistency in copyable_holder_caster: the correct load_value() return type is void (as defined in type_caster_generic).

For easy future reference, this is the long-standing inconsistency:

Noticed in passing while working on PR #5213.


commit 92d7724:

  • Add DANGER ZONE comment in detail/init.h, similar to a comment added on the smart_holder branch (all the way back in 2021).

Suggested changelog entry:

rwgk added 3 commits July 18, 2024 00:41
This is in support of PR pybind#5213:

* trampoline_self_life_support.h depends on value_and_holder.h

* type_caster_base.h depends on trampoline_self_life_support.h
…d on the smart_holder branch (all the way back in 2021).
rwgk added a commit to rwgk/pybind11 that referenced this pull request Jul 18, 2024
…from smart_holder branch almost as-is.

This ensures IWYU correctness in client code.

The only change in trampoline_self_life_support.h is to replace `#include "detail/type_caster_base.h"` with "#include detail/value_and_holder.h".
rwgk added a commit to rwgk/pybind11 that referenced this pull request Jul 18, 2024
@rwgk rwgk changed the title [WIP] Cleanup/Refactoring in support of PR #5213 Small cleanup/refactoring in support of PR #5213 Jul 18, 2024
@rwgk rwgk marked this pull request as ready for review July 18, 2024 16:23
@rwgk rwgk requested a review from henryiii as a code owner July 18, 2024 16:23
@rwgk
Copy link
Collaborator Author

rwgk commented Jul 19, 2024

Thanks @henryiii !

@rwgk rwgk merged commit 6d4805c into pybind:master Jul 19, 2024
86 checks passed
@rwgk rwgk deleted the bakein_prep_on_master branch July 19, 2024 00:34
rwgk added a commit to rwgk/pybind11 that referenced this pull request Jul 19, 2024
…/value_and_holder.h"` in trampoline_self_life_support.h. This was made possible by PR pybind#5251.
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 19, 2024
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Aug 12, 2024
henryiii pushed a commit that referenced this pull request Aug 13, 2024
* Factor out detail/value_and_holder.h (from detail/type_caster_base.h)

This is in support of PR #5213:

* trampoline_self_life_support.h depends on value_and_holder.h

* type_caster_base.h depends on trampoline_self_life_support.h

* Fix a minor and inconsequential inconsistency in `copyable_holder_caster`: the correct `load_value()` return type is `void` (as defined in `type_caster_generic`)

For easy future reference, this is the long-standing inconsistency:

* https://github.com/pybind/pybind11/blob/dbf848aff7c37ef8798bc9459a86193e28b1032f/include/pybind11/detail/type_caster_base.h#L634

* https://github.com/pybind/pybind11/blob/dbf848aff7c37ef8798bc9459a86193e28b1032f/include/pybind11/cast.h#L797

Noticed in passing while working on PR #5213.

* Add `DANGER ZONE` comment in detail/init.h, similar to a comment added on the smart_holder branch (all the way back in 2021).
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.

None yet

2 participants