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 PyString::new_bound #3774

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Conversation

davidhewitt
Copy link
Member

Part of #3684

This implements PyString::new_bound and deprecates PyString::new as per the established pattern, updating internal code.

There are other constructors on PyString, particularly PyString::intern, which I will cover in a separate PR to keep the diff smaller.

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Jan 29, 2024
@davidhewitt davidhewitt force-pushed the string-new-bound branch 2 times, most recently from ddc7769 to b399737 Compare January 29, 2024 11:18
@davidhewitt
Copy link
Member Author

Sorry for the force-pushes here; I'll try to make the next one I push a bit tidier on first push... 🤦

Copy link

codspeed-hq bot commented Jan 29, 2024

CodSpeed Performance Report

Merging #3774 will degrade performances by 29.03%

Comparing davidhewitt:string-new-bound (c475656) with main (7549a21)

Summary

❌ 2 regressions
✅ 78 untouched benchmarks

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

Benchmarks breakdown

Benchmark main davidhewitt:string-new-bound Change
extract_float_downcast_success 418.9 ns 474.4 ns -11.71%
extract_str_extract_success 675 ns 951.1 ns -29.03%

@@ -206,7 +206,7 @@ impl PyErr {
/// assert_eq!(err.to_string(), "TypeError: ");
///
/// // Case #3: Invalid exception value
/// let err = PyErr::from_value(PyString::new(py, "foo").into());
/// let err = PyErr::from_value(PyString::new_bound(py, "foo").as_gil_ref());
Copy link
Member

Choose a reason for hiding this comment

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

This will have to wait for the corresponding PyErr change, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we need to add PyErr::from_value_bound I think.

(or add a trait which abstracts over &PyAny and &Bound<PyAny> just temporarily to ease migration, thought that'd be a new design question and less mechanical.)

@davidhewitt davidhewitt enabled auto-merge January 29, 2024 11:52
@davidhewitt davidhewitt added this pull request to the merge queue Jan 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jan 29, 2024
@davidhewitt davidhewitt enabled auto-merge January 29, 2024 13:14
@davidhewitt davidhewitt added this pull request to the merge queue Jan 29, 2024
Merged via the queue into PyO3:main with commit 0d421b1 Jan 29, 2024
37 of 38 checks passed
@davidhewitt davidhewitt deleted the string-new-bound branch January 29, 2024 14:11
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