Add more IntoPyObject impls
#4446
Conversation
There was a problem hiding this comment.
Overall looks great to me 👍
How should we go about testing these? Probably some level of coverage will be reached once we start migrating off the older traits, though it would be nice to drive that coverage closer to 100% if possible. Coverage got a bit slack with all the migrating off the GIL Refs and the huge pile of churn / deprecations. Maybe I should accept it'll be lower for the next couple of releases and then once we get to a steadier place again, we can work to drive it back up...
| } | ||
| } | ||
|
|
||
| impl<'a, 'py, T: Copy + IntoPyObject<'py>> IntoPyObject<'py> for &'a Cell<T> { |
There was a problem hiding this comment.
This one is interesting; because of the nature of the cell as a copy-on-read container it means that we have T: IntoPyObject instead of &T, despite this being a reference to a container.
If we wanted to keep it consistent with the ToPyObject implementation above (and the general trend), this implementation should probably go through &T. But is that actually correct, or was the original ToPyObject implementation just wrong? 🤔
There was a problem hiding this comment.
Interesting thought... I'd argue that this implementation makes more sense, since we get an owned T out. There would be nothing to tie the lifetime of that reference to (since it is a temporary inside the impl) and we would have to write HRTB to express that. In the end it basically describes the same thing, with a lot more syntax.
I think in the ToPyObject is masks this HRTB kind of because the implementation can't define or restrict the "'self" lifetime and has to be valid for any lifetime.
There was a problem hiding this comment.
Yes, I think I agree that this new implementation is the correct formulation 👍
I definitely agree here, we should strive to keep coverage high. Maybe we can try to implement the |
👍 I think that'd be a good way to reduce the amount of duplication, and then as we rotate out the old traits all that will be left behind is thin wrappers which won't be a huge concern if deprecated & not covered. There might be a few cases where the old traits can't use these new ones (e.g. different bytes specialization behaviour maybe?), where I guess we just live with the duplication. |
True, I guess all generic types can't do this, because of the bounds... Anyway doing it for "leaf" types will be better than not doing at all. |
CodSpeed Performance ReportMerging #4446 will improve performances by 17.36%Comparing Summary
Benchmarks breakdown
|
|
I looks like coverage has improved significantly from that. Now its pretty much just the generic collections (and for a lot of them just the reference) impls that are not covered. (I also found a few impls that I've missed previously, so I added those as well) |
This adds more
IntoPyObjectimpls, mostly for reference types, plus a few that I missed in the first round. I generally tried to provide one where ever we also have aToPyObjectimpl, but there could be still some that I missed somewhere. They will hopefully show up once I start migrating trait bounds.