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

expose Bound::from_owned_ptr etc #3779

Merged
merged 1 commit into from
Feb 5, 2024
Merged

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Jan 29, 2024

Split from #3606

This makes Bound::from_owned_ptr and variants part of the public API, instead of pub(crate).

This is useful for the occasional use case outside of PyO3 which deals with raw FFI. These APIs also have equivalents with the same names which already exist on Py<T>, so it seems to me that we might as well have these.

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Jan 29, 2024
Copy link

codspeed-hq bot commented Jan 29, 2024

CodSpeed Performance Report

Merging #3779 will improve performances by 20.41%

Comparing davidhewitt:bound-from-ptr (86f294f) with main (ecb4ecb)

🎉 Hooray! pytest-codspeed just leveled up to 2.2.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

⚡ 4 improvements
✅ 75 untouched benchmarks

Benchmarks breakdown

Benchmark main davidhewitt:bound-from-ptr Change
list_via_downcast 185 ns 157.2 ns +17.67%
sequence_from_list 355.6 ns 300 ns +18.52%
mapping_from_dict 327.8 ns 272.2 ns +20.41%
not_a_list_via_downcast 242.8 ns 215 ns +12.92%

@davidhewitt
Copy link
Member Author

I suppose the one design question here is whether we think the simpler Bound::from_ptr, Bound::from_ptr_or_opt etc would be better(i.e. dropping the _owned bit).

Nicer names maybe, but they then lose the consistency with Py.

@Icxolu
Copy link
Contributor

Icxolu commented Feb 5, 2024

I think it makes sense to have these as a complement to the ones on Py. I would suggest to keep the names the same. It's not that much longer to keep _owned and I think its easier to reason about for users if the names are consistent.

I would also suggest to deprecate the corresponding functions on Python, which look like the predecessors of these.

@davidhewitt
Copy link
Member Author

Thanks, that's enough support for me to merge forward with these as-is for now 👍

I would also suggest to deprecate the corresponding functions on Python, which look like the predecessors of these.

Agreed, let me see if I can throw together something for those quickly now...

@davidhewitt
Copy link
Member Author

Agreed, let me see if I can throw together something for those quickly now...

Actually there's still quite a lot of work to migrate off these internally, for now I've just added bullets to #3684 under a deprecations section.

@davidhewitt davidhewitt added this pull request to the merge queue Feb 5, 2024
Merged via the queue into PyO3:main with commit 020ed39 Feb 5, 2024
38 checks passed
@davidhewitt davidhewitt deleted the bound-from-ptr branch February 5, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants