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

Change the types of PySliceIndices and `PySlice::indices #3761

Merged
merged 4 commits into from
Apr 18, 2024

Conversation

cmpute
Copy link
Contributor

@cmpute cmpute commented Jan 26, 2024

This fixes #3756. I decided not to change the type of length to usize, to leave the user with the option to skip the check, because isize::MAX is already very large as a length of sequences.

Copy link

codspeed-hq bot commented Jan 26, 2024

CodSpeed Performance Report

Merging #3761 will not alter performance

Comparing cmpute:slice_index (2679a28) with main (c1f11fb)

Summary

✅ 70 untouched benchmarks

@davidhewitt
Copy link
Member

Thanks! Overall I think this is a straightforward thing for users to update for, and makes things a bit tidier, so overall I'm warm to this change.

My only hesitation as I mentioned before is migrating from 0.20 to 0.21 is already set to be a big job for users so I'm a bit wary of adding more breaking changes right at this minute, especially as this doesn't seem strictly necessary.

I'd be happy to hear other maintainer's views here; otherwise if you can forgive me @cmpute, I'd prefer to take a step of caution and save this for 0.22.

@cmpute
Copy link
Contributor Author

cmpute commented Jan 27, 2024

I'm open for the merge timepoint :) Waiting for 0.22 sounds good to me!

@davidhewitt davidhewitt added this to the 0.22 milestone Jan 27, 2024
@cmpute
Copy link
Contributor Author

cmpute commented Apr 2, 2024

I have rebased the code, not sure why the codecov check failed tho.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, sorry to take a while to get to this, let's merge!

The uncovered path looks like it's the old GIL Ref API, we'll live with that for the moment. (Going to be nice in 0.23 when coverage shoots back up!)

@davidhewitt davidhewitt added this pull request to the merge queue Apr 18, 2024
Merged via the queue into PyO3:main with commit 03c50a1 Apr 18, 2024
42 of 43 checks passed
@jessekrubin
Copy link
Contributor

I am using slices, and this is causing me some trouble in upgrading to 0.22. do I HAVE to enable gil-refs just need those slice indices?

@davidhewitt
Copy link
Member

No, you need to use the new PySliceMethods trait.

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.

Change the type of length from c_long to isize (in PySlice::index)
3 participants