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

Make PyTuple constructors return &PyTuple #447

Merged
merged 3 commits into from
May 25, 2019
Merged

Conversation

birkenfeld
Copy link
Member

This has a test failure in test_gc:

---- create_pointers_in_drop stdout ----
thread 'create_pointers_in_drop' panicked at 'assertion failed: `(left == right)`
  left: `2127`,
 right: `2126`', tests/test_gc.rs:123:9
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

Also I'm not sure about the FromPy impl...

@konstin
Copy link
Member

konstin commented Apr 18, 2019

I assume the test calls PyTuple::empty which had returned an owned reference (Py<PyTuble) while &PyTuple isn't owned. Creating an owned reference increases the reference count by 1, which that test expects to happen.

pyo3/tests/test_gc.rs

Lines 96 to 98 in 874d8a0

let _empty1 = PyTuple::empty(py);
let _empty2: PyObject = PyTuple::empty(py).into();
let _empty3: &PyAny = py.from_owned_ptr(ffi::PyTuple_New(0));

@birkenfeld
Copy link
Member Author

Tried to make this equivalent to before using from_py, but still failing...

Copy link
Member

@Alexander-N Alexander-N left a comment

Choose a reason for hiding this comment

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

Added some comments to clear up why the test fails. I'm not sure what this test is trying to assert so I can't say if everything is ok.

@@ -110,7 +111,7 @@ fn create_pointers_in_drop() {
{
let gil = Python::acquire_gil();
let py = gil.python();
let empty = PyTuple::empty(py);
let empty: Py<PyTuple> = FromPy::from_py(PyTuple::empty(py), py);
ptr = empty.as_ptr();
cnt = empty.get_refcnt() - 1;
let inst = Py::new(py, ClassWithDrop {}).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

After calling drop(inst) the refcount of empty is increased by 4. Only 2 of those references survive the end of the scope.

tests/test_gc.rs Outdated
@@ -110,7 +111,7 @@ fn create_pointers_in_drop() {
{
let gil = Python::acquire_gil();
let py = gil.python();
let empty = PyTuple::empty(py);
let empty: Py<PyTuple> = FromPy::from_py(PyTuple::empty(py), py);
ptr = empty.as_ptr();
cnt = empty.get_refcnt() - 1;
Copy link
Member

Choose a reason for hiding this comment

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

PyTuple::empty(py) increases the refcount of the empty tuple by 1 and FromPy::from_py(PyTuple::empty(py), py) by 2. So if Py<PyTuple> is used here for empty (not necessary) then cnt has to be corrected with

cnt = empty.get_refcnt() - 2;

@kngwyu kngwyu marked this pull request as ready for review May 25, 2019 13:12
@kngwyu kngwyu force-pushed the tuple_new branch 2 times, most recently from 3f044df to 30778a2 Compare May 25, 2019 14:05
@codecov
Copy link

codecov bot commented May 25, 2019

Codecov Report

Merging #447 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #447   +/-   ##
======================================
  Coverage    87.8%   87.8%           
======================================
  Files          65      65           
  Lines        3435    3435           
======================================
  Hits         3016    3016           
  Misses        419     419
Impacted Files Coverage Δ
src/types/tuple.rs 90% <100%> (+0.44%) ⬆️
src/conversion.rs 97.34% <100%> (ø) ⬆️
src/instance.rs 96.66% <0%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5a2059...e4287e4. Read the comment docs.

@codecov
Copy link

codecov bot commented May 25, 2019

Codecov Report

Merging #447 into master will decrease coverage by 0.35%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #447      +/-   ##
==========================================
- Coverage    87.8%   87.44%   -0.36%     
==========================================
  Files          65       65              
  Lines        3435     3410      -25     
==========================================
- Hits         3016     2982      -34     
- Misses        419      428       +9
Impacted Files Coverage Δ
src/types/tuple.rs 90% <100%> (+0.44%) ⬆️
src/conversion.rs 95.57% <100%> (-1.77%) ⬇️
src/types/datetime.rs 94.44% <0%> (-5.56%) ⬇️
src/class/iter.rs 77.77% <0%> (-5.56%) ⬇️
src/types/string.rs 88.13% <0%> (-1.7%) ⬇️
src/class/macros.rs 86.8% <0%> (-1.56%) ⬇️
src/err.rs 61.11% <0%> (-1.32%) ⬇️
src/buffer.rs 69.9% <0%> (-0.49%) ⬇️
src/freelist.rs 82.22% <0%> (-0.39%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5a2059...e4287e4. Read the comment docs.

@kngwyu
Copy link
Member

kngwyu commented May 25, 2019

I fixed the tests and examples.
@birkenfeld Thanks for your PR!
@Alexander-N Thanks for your advice on the failing test!

@kngwyu kngwyu merged commit 4d7ca3a into PyO3:master May 25, 2019
@birkenfeld birkenfeld deleted the tuple_new branch March 13, 2020 05:51
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.

4 participants