More precise return_value_policy::automatic documentation. #2920
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
rwgk
force-pushed
the
test_return_vector_bool_raw_ptr
branch
from
9f439ac to
d521169
Compare
rwgk
pushed a commit
to google/clif
that referenced
this pull request
Mar 29, 2021
…o `absltest`. * Background: b/183240258 * Migration CL: cl/363927286 * Initial attempt to fix the ASAN failure for std_containers_test: cl/363992952 This CL takes a different approach compared to cl/363992952 for std_containers_test, by replacing `new SomeType` with `static SomeType`, and returning a pointer to the statically allocated object. `std_containers.h` is revised accordingly in this CL. A detail regarding the `std::vector<bool> *` return in std_containers.h: currently the pybind11 behavior does not match the documentation in pybind11/docs/advanced/functions.rst. pybind11 does NOT `take_ownership` if no `return_value_policy` is specified (see [pybind11 PR #2920](pybind/pybind11#2920)). Therefore this CL is correct. TODO for a separate CL: change the pybind11 code generator to specify `return_value_policy::reference`, to be explicit and leave no room for doubt. PiperOrigin-RevId: 365616794
rwgk
force-pushed
the
test_return_vector_bool_raw_ptr
branch
from
d521169 to
36eda14
Compare
rwgk
force-pushed
the
test_return_vector_bool_raw_ptr
branch
from
5613cce to
efee5df
Compare
rwgk
force-pushed
the
test_return_vector_bool_raw_ptr
branch
from
2882871 to
fbf35ed
Compare
rwgk
changed the title
henryiii
reviewed
Jul 12, 2021
tests/test_stl.py
Outdated
Comment on lines
289
to
293
| while True: | ||
| v = m.return_vector_bool_raw_ptr() | ||
| assert isinstance(v, list) | ||
| assert len(v) == 4513 | ||
| break # Comment out for manual leak checking (use `top` command). |
There was a problem hiding this comment.
Kind of odd to have in a test? Maybe
Suggested change
| while True: | |
| v = m.return_vector_bool_raw_ptr() | |
| assert isinstance(v, list) | |
| assert len(v) == 4513 | |
| break # Comment out for manual leak checking (use `top` command). | |
| # Add while True: for manual leak checking | |
| v = m.return_vector_bool_raw_ptr() | |
| assert isinstance(v, list) | |
| assert len(v) == 4513 |
There was a problem hiding this comment.
Done. Thanks Henry!
Side remark & question:
- I have a lot of those on the smart_holder branch, and need to turn them on-off quite a bit when going into the smart_holder code.
- Do you happen to be aware of a pytest feature or plugin that would automate the fool-proof top-like leak check with adding a certain flag?
I've been thinking of this as a potential Google Summer of Code project if it doesn't exist already. Roughly: run decorated test functions in a loop while monitoring the equivalent of top's RES from within the same Python process, use some simple heuristics to decide "this could be a leak" or "no this doesn't look like a leak after running for N seconds".
henryiii
approved these changes
Jul 12, 2021
added 4 commits
July 12, 2021 15:24
… to be comprehensive, to not bloat the reference table with too many details.
rwgk
force-pushed
the
test_return_vector_bool_raw_ptr
branch
from
fbf35ed to
4ff6527
Compare
|
I decided to remove the needs changelog label. It's an important detail to have right for someone looking carefully at the docs, but I feel that highlighting this PR in the changelog gives it too much weight (more distracting than helpful). |
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.
I wrote the test under this PR to convince myself that the existing documentation is not actually precise. I think it's nice to keep the test as a reference.