Skip to content

Bug fix: adding back !is_alias<Class>(ptr) that were accidentally omitted. #2958

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

Merged
merged 7 commits into from
Apr 19, 2021

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Apr 18, 2021

This PR started as a small bug fix for include/pybind11/detail/init.h.

The init.h bug escaped detection because of missing test coverage in test_factory_constructors. To systematically plug such holes, the PYBIND11_SH_DEF macro was introduced and applied to test_factory_constructors, test_multiple_inheritance, and test_methods_and_attributes.

To have a complete toolset for Classic / Conservative / Progressive transitions, the PYBIND11_SH_AVL macro was added. test_mock_cpp and README_smart_holder.rst were revised accordingly.

@rwgk rwgk requested a review from henryiii as a code owner April 19, 2021 15:05
Comment on lines -26 to +27
- 3.10-dev
# Broken b/o https://github.com/pytest-dev/pytest/issues/8539
# - 3.10-dev
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fine to me until this gets resolved.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, this would be much better as it's own PR, not mixed into an unrelated PR. Also makes it easier to revert.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can cherry-pick b3fff90 I think we can accept it immediately and then restart the CI (or just ignore these jobs) for the pending PRs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @henryiii, thanks, I'm not sure how obvious this is in your view, this is only the smart_holder branch. (Pointing out to make sure you're not surprised.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super obvious, that explains it; worth cherry-picking and applying to master via a PR, I think.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does help explain the smart-holder PRs. Maybe a labeler or prefix (like [smart holder]) or such would help. Or I can just watch for the target branch. :)

@rwgk
Copy link
Collaborator Author

rwgk commented Apr 19, 2021

The CI is green, also with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT (#2879). This change will get reviewed Google-internally. Merging. I'll open a new PR if there are requests for changes.

@rwgk rwgk merged commit 99de498 into pybind:smart_holder Apr 19, 2021
@rwgk rwgk deleted the sh_init_h_bug_fix branch April 19, 2021 17:54
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Apr 19, 2021
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Apr 19, 2021
rwgk added a commit to google/clif that referenced this pull request Apr 20, 2021
The significant PRs are:
* pybind/pybind11#2947 — Bug fix: `trampoline_self_life_support` CpCtor, MvCtor.
* pybind/pybind11#2958 — Bug fix: adding back `!is_alias<Class>(ptr)` that were accidentally omitted.

These PRs are of the more-surgical kind and probably difficult to review. However, README_smart_holder.rst has gone through a couple significant iterations now, a careful top-to-bottom review could be very helpful (and also build valuable background for reviewing the code and test changes). README_smart_holder.rst is most easily reviewed here: https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst

Note that the smart_holder branch itself is kept up-to-date with master. A few commits listed below are the result of `git merge master`.

TESTED=TGP (http://tap/OCL:368110073:BASE:369379217:1618900855748:76fc103)

Import pybind/pybind11 from GitHub.

  - e0c1dadb757296c8bc91760a62bfb48b48c39530 chore: add myself to CODEOWNERS (#2940) by Henry Schreiner <HenrySchreinerIII@gmail.com>
  - 8efd5e3820c8856f65030f3568eccd471f274efe Bug fix: trampoline_self_life_support CpCtor, MvCtor. (#2... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - 6709abba934e968d31c24f20c132c67a6f424a6a Allow function pointer extraction from overloaded functio... by Tamaki Nishino <otamachan@gmail.com>
  - 62976cfcb8ccc76a79849028d7f5a3f8c7019007 fix: using -Werror-all for Intel (#2948) by Philipp Bucher <philipp.bucher@tum.de>
  - 49f8f60ec4254e0f55db3c095c945210bcb43ad2 Adding documentation with associated test: Using py::clas... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - 793adbda509d754551a4c0869525ef9ac81c2955 Revert "Adding documentation with associated test: Using ... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - cf89b865bbf4fcf13adfc5521c18eb9b1ec12c9e Adding documentation with associated test: Using py::clas... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - ab590c624bca471e16dc1d9482468d5f2e2de4d9 Disabling deadsnakes 3.10-dev CI (currently broken). by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - 1c8795a2050dff364688ef802064358f9dca3d3a Changing PYBIND11_SMART_HOLDER_TYPE_CASTERS to use __VA_A... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - 114be7f4ade0ad798cd4c7f5d65ebe4ba8bd892d docs: remove recommonmark (#2955) by Henry Schreiner <HenrySchreinerIII@gmail.com>
  - d368b728812dd85168b68afa38cc584a0e436f66 Connecting PYBIND11_INTERNALS_VERSION to PYBIND11_USE_SMA... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - e08a58111dbea38d667b209f7543864d51a3b185 Fix compilation with gcc < 5 (#2956) by mvoelkle-cern <54059203+mvoelkle-cern@users.noreply.github.com>
  - 99de498b26f68d212148c45de919d96d1121a4a3 Bug fix: adding back `!is_alias<Class>(ptr)` that were ac... by Ralf W. Grosse-Kunstleve <rwgk@google.com>

PiperOrigin-RevId: 369447165
@EricCousineau-TRI EricCousineau-TRI added the smart holder See: https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst label Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
smart holder See: https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants