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.

Sign up for GitHub

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

Jump to bottom

Move PyAnyMethods::downcast to Bound smart pointer? #3980

Open
davidhewitt opened this issue Mar 22, 2024 · 1 comment · May be fixed by #3988
Open

Move PyAnyMethods::downcast to Bound smart pointer? #3980

davidhewitt opened this issue Mar 22, 2024 · 1 comment · May be fixed by #3988

Comments

@davidhewitt
Copy link
Member

Just testing out the new 0.21 beta in pythonize, and it turns out that using obj.downcast_into() on PyAnyMethods for Bound<T> where T isn't PyAny is slightly cumbersome because the self receiver means accessing the method via deref doesn't work.

Instead using .into_any().downcast_into() chain is necessary.

Moving PyAnyMethods::downcast_into to be on Bound<T> instead would avoid the .into_any() call. This is a rare case, and the drawback would be extra generic code bloat by having all the .downcast() implementations for many different Bound<T> types.

Overall I didn't feel strongly and felt like this could be up for discussion. I think we could add this in 0.22 if we don't make a decision for 0.21.
@davidhewitt davidhewitt mentioned this issue Mar 22, 2024
Closed
@adamreichold
Copy link
Member

the drawback would be extra generic code bloat by having all the .downcast() implementations for many different Bound types.

We would still see an increase in LLVM lines, but I guess this could still forward to a non-generic implementation and be #[inline(always)] itself to be compiled away as completely as possible (actually already from the MIR now that we have a MIR inliner IIRC). So I would not worry about that too much if it can measurably improve library ergonomics.

@davidhewitt davidhewitt linked a pull request Mar 23, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

move downcast functions from PyAnyMethods to Bound<T> davidhewitt/pyo3
2 participants
@davidhewitt @adamreichold