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

add Bound API constructors from borrowed pointers #3858

Merged
merged 4 commits into from
Feb 18, 2024

Conversation

davidhewitt
Copy link
Member

As per #3708 (comment)

I decided to add both Bound::from_borrowed_ptr and Borrowed::from_ptr sets of methods. I think the Bound methods make sense from a consistency with Py angle and also for simplicity. The Borrowed methods may be useful in extreme cases for power users, and also are what I'd like to use internally in #3708

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

codspeed-hq bot commented Feb 18, 2024

CodSpeed Performance Report

Merging #3858 will degrade performances by 20.41%

Comparing davidhewitt:borrowed-ptrs (bc53537) with main (5f42c02)

Summary

⚡ 3 improvements
❌ 1 regressions
✅ 75 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main davidhewitt:borrowed-ptrs Change
f64_from_pyobject 516.7 ns 405.6 ns +27.4%
list_via_downcast 185 ns 157.2 ns +17.67%
list_via_extract 364.4 ns 308.9 ns +17.99%
not_a_list_via_downcast 216.7 ns 272.2 ns -20.41%

Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

It seems reasonable to me, that we add both Bound and Borrowed variants. Found a small documentation mixup, otherwise LGTM

src/instance.rs Outdated
@@ -480,33 +516,52 @@ impl<'py, T> Borrowed<'_, 'py, T> {
}

impl<'a, 'py> Borrowed<'a, 'py, PyAny> {
/// Constructs a new `Borrowed<'py, PyAny>` from a pointer. Panics if `ptr` is null.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Constructs a new `Borrowed<'py, PyAny>` from a pointer. Panics if `ptr` is null.
/// Constructs a new `Borrowed<'a, 'py, PyAny>` from a pointer. Returns an `Err` by calling `PyErr::fetch`
/// if `ptr` is null.

Missing lifetime here, same for the others. Also you mixed the or_err and panicking variants

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I think I got mixed up because the functions aren't in the same order as for Bound - I'll swap them so that from_ptr is first at the same time 👍

@davidhewitt
Copy link
Member Author

Thanks for the review, will try to fixup tonight 👍

@davidhewitt davidhewitt added this pull request to the merge queue Feb 18, 2024
Merged via the queue into PyO3:main with commit a85ed34 Feb 18, 2024
36 of 39 checks passed
@davidhewitt davidhewitt deleted the borrowed-ptrs branch February 18, 2024 23:33
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