Conversation
birkenfeld
force-pushed
the
pylist_insert_index
branch
from
f4d4750 to
94855b1
Compare
birkenfeld
changed the title
birkenfeld
force-pushed
the
pylist_insert_index
branch
2 times, most recently
from
5c54299 to
ed51694
Compare
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Hmm, staring at this, I now think that we need to be careful when casting the usize function arguments to Py_ssize_t...
|
Yeah that's a good point. I think the simplest way would be clamping to |
…rt and PyList::set_item. NB: the behavior on out-of-range indices hasn't changed; it was merely wrongly documented before. See #1667
birkenfeld
force-pushed
the
pylist_insert_index
branch
from
ed51694 to
79d9741
Compare
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
👍 needs a CHANGELOG entry, I think, otherwise LGTM!
It might be worth adding tests with out-of-range usize values, now that we've realised that edge case...
|
|
||
| /// Takes a slice of the tuple from `low` to the end and returns it as a new tuple. | ||
| pub fn split_from(&self, low: isize) -> &PyTuple { | ||
| pub fn split_from(&self, low: usize) -> &PyTuple { |
There was a problem hiding this comment.
Looks like this function lacks a test, so it would be great to add one if you're willing!
There was a problem hiding this comment.
Actually I was wondering if it's needed. The operation exists neither in Python nor in Rust, is not used internally, and it's easy enough to use slice instead.
There was a problem hiding this comment.
Fair point. Yes let's deprecate it and plan to remove.
As you say, it's easy to reproduce with either .slice() or .as_slice().split_at()
|
I was thinking that this should have a common changelog entry with #1733 and the other followups. BTW, is |
Not really sure where it fits either. I think it's fine there for now. As it's |
|
It looks like this is unlikely to cause much pain when cherry-picking other things from |
2 participants
NB: the behavior on out-of-range indices hasn't changed;
it was merely wrongly documented before.
If we want to panic instead, to match e.g. Vec::insert, this needs to be implemented specially.