[smart_holder] .def_readonly, .def_readwrite adaptors (continuation of PR #3581). #3844
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
May 13, 2022 07:08
… as suggested by @charlesbeattie back in Jan/Feb under PR pybind#3581.
…rty (as suggested by @rainwoodman)
rwgk
force-pushed
the
sh_property_base_2022-04-01
branch
from
d5a958a
to
8ec5721
Compare
|
@rainwoodman Thanks for the comments on PR #3581, and sorry for taking extremely long to get back to it. I closed the other PR and continued with this one because we did the global clang-formatting in the meantime. I think I addressed all comments, in separate small commits here, hoping that makes it easy for you to re-review. If you get a chance, could you please take another look? Re CI: we have two existing failures on master, therefore 4 failures here, 2 for CI (smart_holder is just an add-on), 2 for CI-SH-DEF (smart_holder is the default holder). |
Skylion007
reviewed
May 14, 2022
Skylion007
reviewed
May 14, 2022
|
The 4 CI failures are still the 2 x 2 failures that were fixed only recently on master. I'll go ahead and merge this PR now, as a starting-point for updating the smart_holder branch (git merge master), global testing, and importing into the Google environment. Thanks for all the feedback! (If you have any additional suggestions, I'll work on them in a new PR.) |
wangxf123456
pushed a commit
to google/clif
that referenced
this pull request
Jun 4, 2022
* Includes pybind/pybind11#3844 — [smart_holder] .def_readonly, .def_readwrite adaptors * Also removing no-longer-needed workaround from third_party/clif/testing/python/nested_fields_test.py * third_party/pybind11/google3_patches/stl_h_include_iostream.patch undoes pybind/pybind11#3928, to unblock this CL. The patch will be removed after missing `#include <iostream>` are added to ~5 projects that currently have a transitive dependency through pybind11 (for broken targets fixed by this patch see https://fusion2.corp.google.com/presubmit/tap/449354800/OCL:449354800:BASE:449423292:1652863858736:bd18ffcd;groups=Passing/targets). - 2e331308d38a521c087e7fc0cfee227cd29f3f71 chore: remove unused include from stl.h (#3928) by Aaron Gokaslan <skylion.aaron@gmail.com> - ad146b2a1877e8ba3803f94a7837969835a297a7 [pre-commit.ci] pre-commit autoupdate (#3933) by pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> - 5621ab853a60ad48bce08487cc6e220930178b79 Do we have a unit test for the traceback code in error_st... by Ralf W. Grosse-Kunstleve <rwgk@google.com> - 48c7be4a5643cdf48a1228de05f6279ec95e99d3 Undoing previous accidental commit. Sorry I forgot to git... by Ralf W. Grosse-Kunstleve <rwgk@google.com> - 72eea20afd51e363fe115265043c2b2b6bcc523a Fix py::cast from pytype rvalue to pytype (#3949) by Maarten Baert <maarten-baert@hotmail.com> - 1a7b12983e09f698be3007b5868bfdf931d9a4d1 ci: fix cuda issue & MSVC spurious warning (#3950) by Henry Schreiner <HenrySchreinerIII@gmail.com> - dff6fa0554bf6efe98a8da3f932b749cff4d76a8 fix(cmake): avoid issue with NVCC + Windows (#3947) by Henry Schreiner <HenrySchreinerIII@gmail.com> - a8b3ff30f9649459021adc80f98a945d3ac675a5 chore: add a couple of moves in pybind11.h (#3941) by Aaron Gokaslan <skylion.aaron@gmail.com> - d28c3a5da7a199530be017000ba4dfb2ff812624 [smart_holder] .def_readonly, .def_readwrite adaptors (co... by Ralf W. Grosse-Kunstleve <rwgk@google.com> - 50220aeb09ac6ae02b6dd95fdc78f7c537920068 Tracking ci.yml changes from master. by Ralf W. Grosse-Kunstleve <rwgk@google.com> PiperOrigin-RevId: 449521012
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Note: This PR is a continuation of PR #3581, to preserve the original development history. The large-scale changes from Feb 2022 (Python 2 drop, global clang-format) make it difficult to meaningfully preserve the original development history in the same PR.
The original motivation for this PR was to fix the issue observed here: disowning creates a dangling pointer.
To ensure internal consistency down the road, the scope of this PR was expanded to cover
.def_readonlyand.def_readwritebehavior forstd::unique_ptrstd::shared_ptrmembers (mutable & const for the pointer types).
For
shared_ptrmembers, the existing behavior is as desired (no work needed).For value and raw-pointer members, the PyCLIF approach here was adopted: aliasing
shared_ptrtying the lifetime of the "outer" object to the lifetime of the Python object pointing to the member. In this way the existing mechanisms prevent the "outer" object from being disowned while pointers to members are still alive.For unique_ptr members, the situation is complex. This PR only establishes a proof of concept. See the code and comments in tests/test_class_sh_property.py for details. Productizing the proof of concept is left for a future separate PR, as needed.