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 constructor for PyBool #3790

Merged
merged 1 commit into from
Feb 3, 2024
Merged

add Bound constructor for PyBool #3790

merged 1 commit into from
Feb 3, 2024

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Feb 2, 2024

Part of #3684

This adds the Bound constructors for PyBool.

Since the ffi types Py_True and Py_False are singletons pointing into static memory, this return Borrowed instead of Bound. I choose to still keep the name new_bound for consistency with the other type. Since Borrowed derefs into &Bound it also behaves very similarly.

src/instance.rs Show resolved Hide resolved
src/impl_/pyclass.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

Thank you yet again 👍

I'm a bit under the weather this evening so I'll do my best to read and review first thing in the morning.

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Feb 3, 2024
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, this looks like the right design to me! I think the __ne__ implementation probably needs to create a "new reference" using .to_owned(), otherwise I have mostly just added comments of approval and very tiny nits.

src/types/boolobject.rs Show resolved Hide resolved
src/instance.rs Show resolved Hide resolved
src/types/any.rs Outdated Show resolved Hide resolved
src/impl_/pyclass.rs Outdated Show resolved Hide resolved
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 again, looks good to me!

We're getting pretty close to having this API be feature-complete now! I think there will still be a long tail of adjustments and refactoring, but users can hopefully start playing with this before long 🚀

@davidhewitt davidhewitt added this pull request to the merge queue Feb 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 3, 2024
@davidhewitt davidhewitt added this pull request to the merge queue Feb 3, 2024
Merged via the queue into PyO3:main with commit d8c5e79 Feb 3, 2024
37 of 38 checks passed
/// This returns a [`Borrowed`] reference to one of Pythons `True` or
/// `False` singletons
#[inline]
pub fn new_bound(py: Python<'_>, val: bool) -> Borrowed<'_, '_, Self> {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for being late to the party: If these are singletons, shouldn't the first lifetime even be 'static, i.e.

pub fn new_bound(py: Python<'py>, val: bool) -> Borrowed<'static, 'py, Self>;

or are they still bound to the interpreter?

Copy link
Member

Choose a reason for hiding this comment

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

I wondered about this, I think while static is possibly fine I preferred having this limited to the interpreter. Just in case this becomes relevant for things like subinterpreters in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can always expand this later if users have a need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this too, but it seemed safer to go with this option now, and relax it in the future if possible.

@Icxolu Icxolu deleted the bool branch February 4, 2024 17:38
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.

None yet

3 participants