Skip to content

Commit

Permalink
- PyList, PyTuple and PySequence's get_item now accepts only …
Browse files Browse the repository at this point in the history
…`usize` indices instead of `isize`.

- `PyList` and `PyTuple`'s `get_item` now always uses the safe API. See `get_item_unchecked` for retrieving index without checks.
  • Loading branch information
Aviram Hassan committed Aug 13, 2021
1 parent 388c255 commit 8cbf9f9
Show file tree
Hide file tree
Showing 9 changed files with 258 additions and 120 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Expand Up @@ -8,9 +8,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

## [Unreleased]

### Added
- Add `PyList::get_item_unchecked()` and `PyTuple::get_item_unchecked()` to get items without bounds checks. [#1733](https://github.com/PyO3/pyo3/pull/1733)

### Changed

- Change `PyErr::fetch()` to return `Option<PyErr>`. [#1717](https://github.com/PyO3/pyo3/pull/1717)
- `PyList`, `PyTuple` and `PySequence`'s `get_item` now accepts only `usize` indices instead of `isize`. [#1733](https://github.com/PyO3/pyo3/pull/1733)

### Fixed

Expand Down Expand Up @@ -39,6 +43,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- Fix compiler warning: deny trailing semicolons in expression macro. [#1762](https://github.com/PyO3/pyo3/pull/1762)
- Fix incorrect FFI definition of `Py_DecodeLocale`. The 2nd argument is now `*mut Py_ssize_t` instead of `Py_ssize_t`. [#1766](https://github.com/PyO3/pyo3/pull/1766)

### Changed
- `PyList` and `PyTuple`'s `get_item` now return `PyResult<&PyAny> instead of panicking. [#1733](https://github.com/PyO3/pyo3/pull/1733)
## [0.14.1] - 2021-07-04

### Added
Expand Down
18 changes: 17 additions & 1 deletion benches/bench_list.rs
Expand Up @@ -32,7 +32,22 @@ fn list_get_item(b: &mut Bencher) {
let mut sum = 0;
b.iter(|| {
for i in 0..LEN {
sum += list.get_item(i as isize).extract::<usize>().unwrap();
sum += list.get_item(i).unwrap().extract::<usize>().unwrap();
}
});
}

fn list_get_item_unchecked(b: &mut Bencher) {
let gil = Python::acquire_gil();
let py = gil.python();
const LEN: usize = 50_000;
let list = PyList::new(py, 0..LEN);
let mut sum = 0;
b.iter(|| {
for i in 0..LEN {
unsafe {
sum += list.get_item_unchecked(i).extract::<usize>().unwrap();
}
}
});
}
Expand All @@ -41,6 +56,7 @@ fn criterion_benchmark(c: &mut Criterion) {
c.bench_function("iter_list", iter_list);
c.bench_function("list_new", list_new);
c.bench_function("list_get_item", list_get_item);
c.bench_function("list_get_item_unchecked", list_get_item_unchecked);
}

criterion_group!(benches, criterion_benchmark);
Expand Down
18 changes: 17 additions & 1 deletion benches/bench_tuple.rs
Expand Up @@ -32,7 +32,22 @@ fn tuple_get_item(b: &mut Bencher) {
let mut sum = 0;
b.iter(|| {
for i in 0..LEN {
sum += tuple.get_item(i).extract::<usize>().unwrap();
sum += tuple.get_item(i).unwrap().extract::<usize>().unwrap();
}
});
}

fn tuple_get_item_unchecked(b: &mut Bencher) {
let gil = Python::acquire_gil();
let py = gil.python();
const LEN: usize = 50_000;
let tuple = PyTuple::new(py, 0..LEN);
let mut sum = 0;
b.iter(|| {
for i in 0..LEN {
unsafe {
sum += tuple.get_item_unchecked(i).extract::<usize>().unwrap();
}
}
});
}
Expand All @@ -41,6 +56,7 @@ fn criterion_benchmark(c: &mut Criterion) {
c.bench_function("iter_tuple", iter_tuple);
c.bench_function("tuple_new", tuple_new);
c.bench_function("tuple_get_item", tuple_get_item);
c.bench_function("tuple_get_item_unchecked", tuple_get_item_unchecked);
}

criterion_group!(benches, criterion_benchmark);
Expand Down
2 changes: 1 addition & 1 deletion pyo3-macros-backend/src/from_pyobject.rs
Expand Up @@ -243,7 +243,7 @@ impl<'a> Container<'a> {
for i in 0..len {
let error_msg = format!("failed to extract field {}.{}", quote!(#self_ty), i);
fields.push(quote!(
s.get_item(#i).extract().map_err(|inner| {
s.get_item(#i).and_then(PyAny::extract).map_err(|inner| {
let py = pyo3::PyNativeType::py(obj);
let new_err = pyo3::exceptions::PyTypeError::new_err(#error_msg);
new_err.set_cause(py, Some(inner));
Expand Down
2 changes: 1 addition & 1 deletion src/conversions/array.rs
Expand Up @@ -60,7 +60,7 @@ mod min_const_generics {
if seq_len != N {
return Err(invalid_sequence_length(N, seq_len));
}
array_try_from_fn(|idx| seq.get_item(idx as isize).and_then(PyAny::extract))
array_try_from_fn(|idx| seq.get_item(idx).and_then(PyAny::extract))
}

// TODO use std::array::try_from_fn, if that stabilises:
Expand Down
34 changes: 17 additions & 17 deletions src/types/dict.rs
Expand Up @@ -628,23 +628,23 @@ mod tests {
#[test]
fn test_items() {
Python::with_gil(|py| {
let mut v = HashMap::new();
v.insert(7, 32);
v.insert(8, 42);
v.insert(9, 123);
let ob = v.to_object(py);
let dict = <PyDict as PyTryFrom>::try_from(ob.as_ref(py)).unwrap();
// Can't just compare against a vector of tuples since we don't have a guaranteed ordering.
let mut key_sum = 0;
let mut value_sum = 0;
for el in dict.items().iter() {
let tuple = el.cast_as::<PyTuple>().unwrap();
key_sum += tuple.get_item(0).extract::<i32>().unwrap();
value_sum += tuple.get_item(1).extract::<i32>().unwrap();
}
assert_eq!(7 + 8 + 9, key_sum);
assert_eq!(32 + 42 + 123, value_sum);
});
let mut v = HashMap::new();
v.insert(7, 32);
v.insert(8, 42);
v.insert(9, 123);
let ob = v.to_object(py);
let dict = <PyDict as PyTryFrom>::try_from(ob.as_ref(py)).unwrap();
// Can't just compare against a vector of tuples since we don't have a guaranteed ordering.
let mut key_sum = 0;
let mut value_sum = 0;
for el in dict.items().iter() {
let tuple = el.cast_as::<PyTuple>().unwrap();
key_sum += tuple.get_item(0).unwrap().extract::<i32>().unwrap();
value_sum += tuple.get_item(1).unwrap().extract::<i32>().unwrap();
}
assert_eq!(7 + 8 + 9, key_sum);
assert_eq!(32 + 42 + 123, value_sum);
});
}

#[test]
Expand Down

0 comments on commit 8cbf9f9

Please sign in to comment.