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] Fix HAVE vs HAS naming mishap (PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT) #5286

Merged
merged 1 commit into from
Aug 4, 2024

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Aug 4, 2024

Description

Systematic, trivial name change:

-PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT
+PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT

This is for internal consistency. There are no PYBIND11_HAVE macros, but 10 unique PYBIND11_HAS macros:

$ git grep 'define PYBIND11_HAS' | sed 's/.*define PYBIND11_HAS/PYBIND11_HAS/' | cut -d' ' -f1 | sort | uniq
PYBIND11_HAS_EXPERIMENTAL_FILESYSTEM
PYBIND11_HAS_EXP_OPTIONAL
PYBIND11_HAS_FILESYSTEM
PYBIND11_HAS_FILESYSTEM_IS_OPTIONAL
PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT
PYBIND11_HAS_OPTIONAL
PYBIND11_HAS_STD_LAUNDER
PYBIND11_HAS_STRING_VIEW
PYBIND11_HAS_U8STRING
PYBIND11_HAS_VARIANT

Suggested changelog entry:

@rwgk rwgk marked this pull request as ready for review August 4, 2024 16:16
@rwgk rwgk merged commit 7c6fe49 into pybind:smart_holder Aug 4, 2024
138 checks passed
@rwgk rwgk deleted the have_to_has_sh branch August 4, 2024 16:16
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 4, 2024
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Aug 4, 2024
copybara-service bot pushed a commit to pybind/pybind11_protobuf that referenced this pull request Aug 4, 2024
…ND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT`)

pybind/pybind11#5286

Systematic, trivial name change:

```diff
-PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT
+PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT
```

This is for internal consistency. There are no `PYBIND11_HAVE` macros, but 10 unique `PYBIND11_HAS` macros:

```
$ git grep 'define PYBIND11_HAS' | sed 's/.*define PYBIND11_HAS/PYBIND11_HAS/' | cut -d' ' -f1 | sort | uniq
```
```
PYBIND11_HAS_EXPERIMENTAL_FILESYSTEM
PYBIND11_HAS_EXP_OPTIONAL
PYBIND11_HAS_FILESYSTEM
PYBIND11_HAS_FILESYSTEM_IS_OPTIONAL
PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT
PYBIND11_HAS_OPTIONAL
PYBIND11_HAS_STD_LAUNDER
PYBIND11_HAS_STRING_VIEW
PYBIND11_HAS_U8STRING
PYBIND11_HAS_VARIANT
```

PiperOrigin-RevId: 659316612
copybara-service bot pushed a commit to pybind/pybind11_protobuf that referenced this pull request Aug 5, 2024
…ND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT`)

pybind/pybind11#5286

Systematic, trivial name change:

```diff
-PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT
+PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT
```

This is for internal consistency. There are no `PYBIND11_HAVE` macros, but 10 unique `PYBIND11_HAS` macros:

```
$ git grep 'define PYBIND11_HAS' | sed 's/.*define PYBIND11_HAS/PYBIND11_HAS/' | cut -d' ' -f1 | sort | uniq
```
```
PYBIND11_HAS_EXPERIMENTAL_FILESYSTEM
PYBIND11_HAS_EXP_OPTIONAL
PYBIND11_HAS_FILESYSTEM
PYBIND11_HAS_FILESYSTEM_IS_OPTIONAL
PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT
PYBIND11_HAS_OPTIONAL
PYBIND11_HAS_STD_LAUNDER
PYBIND11_HAS_STRING_VIEW
PYBIND11_HAS_U8STRING
PYBIND11_HAS_VARIANT
```

PiperOrigin-RevId: 659316612
copybara-service bot pushed a commit to pybind/pybind11_protobuf that referenced this pull request Aug 5, 2024
…ND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT`)

pybind/pybind11#5286

Systematic, trivial name change:

```diff
-PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT
+PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT
```

This is for internal consistency. There are no `PYBIND11_HAVE` macros, but 10 unique `PYBIND11_HAS` macros:

```
$ git grep 'define PYBIND11_HAS' | sed 's/.*define PYBIND11_HAS/PYBIND11_HAS/' | cut -d' ' -f1 | sort | uniq
```
```
PYBIND11_HAS_EXPERIMENTAL_FILESYSTEM
PYBIND11_HAS_EXP_OPTIONAL
PYBIND11_HAS_FILESYSTEM
PYBIND11_HAS_FILESYSTEM_IS_OPTIONAL
PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT
PYBIND11_HAS_OPTIONAL
PYBIND11_HAS_STD_LAUNDER
PYBIND11_HAS_STRING_VIEW
PYBIND11_HAS_U8STRING
PYBIND11_HAS_VARIANT
```

PiperOrigin-RevId: 659556864
rwgk added a commit to rwgk/pybind11 that referenced this pull request Mar 1, 2025
This define was
* introduced with pybind#5286
* removed with pybind#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.
rwgk added a commit that referenced this pull request Mar 5, 2025
* 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>
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