Migrate PyTuple / PyList constructors
#4580
Conversation
Icxolu
force-pushed
the
migrate-sequence
branch
2 times, most recently
from
dd87350
to
76f2250
Compare
|
Makes sense 👍 |
src/types/list.rs
Outdated
| type Error = Infallible; | ||
|
|
||
| fn into_pyobject(self, py: Python<'py>) -> Result<Self::Output, Self::Error> { | ||
| self.clone().0.into_pyobject(py) |
There was a problem hiding this comment.
I wonder whether it's simpler to remove Clone from the picture and instead just have the panic produced in here?
IIRC, the main point of the test is to check that panics while constructing the list free stuff properly, Clone was probably just the bug report which came in or similar.
There was a problem hiding this comment.
Yes, I think that makes it a bit clearer. I changed this, removed the now unnecessary cfg and renamed the test (also for PyTuple)
src/types/list.rs
Outdated
| } | ||
|
|
||
| #[inline] | ||
| #[track_caller] | ||
| pub(crate) fn try_new_from_iter<'py>( | ||
| py: Python<'py>, | ||
| elements: &mut dyn ExactSizeIterator<Item = PyResult<PyObject>>, | ||
| elements: &mut dyn ExactSizeIterator<Item = PyResult<Bound<'py, PyAny>>>, |
There was a problem hiding this comment.
Do you think it's worth making these have a concrete iterator? I forget why we had to change it for sets 🤔
There was a problem hiding this comment.
Do you mean impl instead of dyn here? Or what concrete type are you referring to?
Icxolu
force-pushed
the
migrate-sequence
branch
from
e77511b
to
3eddf7c
Compare
Icxolu
force-pushed
the
migrate-sequence
branch
from
3eddf7c
to
42b6c12
Compare
Followup to #4559
PyList::newandPyTuple::newbecome fallible.It's worth double checking the
bad_clone_mem_leakstests, to make sure I didn't accidentally change the semantics there.