PyCFunction bound api

[snuderl]

Part of #3684

Adds _bound versions of PyCFunction constructors.

@davidhewitt Any suggestions what to do with wrap_pyfunction? The change to update it to Bound is pretty minimal, but potentially breaking if people are using it outside the standard m.add_function(wrap_pyfunction!...) scenario

[codspeed-hq]

CodSpeed Performance Report

Merging #3800 will degrade performances by 15.02%

_{Comparing snuderl:PyCFunction-bound-api (aa3c938) with main (059e485)}

Summary

⚡ 3 improvements
❌ 1 regressions
✅ 75 untouched benchmarks

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

Benchmarks breakdown

	Benchmark	main	snuderl:PyCFunction-bound-api	Change
⚡	sequence_from_list	355.6 ns	300 ns	+18.52%
❌	list_via_downcast	157.2 ns	185 ns	-15.02%
⚡	not_a_list_via_downcast	242.8 ns	215 ns	+12.92%
⚡	mapping_from_dict	327.8 ns	272.2 ns	+20.41%

[davidhewitt]

Thanks, overall looks good to me! Just two small style requests...

Regarding wrap_pyfunction, I'm definitely open to trying to see what happens if we change it to return Bound<'py, PyCFunction>. I think that we would potentially need to change both PyModule::add_function and PyModuleMethods::add_function to just take Bound<'py, PyCFunction>.

That's breaking, but hopefully most users would never notice the change if they're using the standard pattern. We'd need to mention this in the migration guide where we also comment on how we made the same decision to change the return type of the intern! macro.

Definitely worth experimenting in its own PR, though.

[snuderl]

Thanks, overall looks good to me! Just two small style requests...

Regarding wrap_pyfunction, I'm definitely open to trying to see what happens if we change it to return Bound<'py, PyCFunction>. I think that we would potentially need to change both PyModule::add_function and PyModuleMethods::add_function to just take Bound<'py, PyCFunction>.

That's breaking, but hopefully most users would never notice the change if they're using the standard pattern. We'd need to mention this in the migration guide where we also comment on how we made the same decision to change the return type of the intern! macro.

Definitely worth experimenting in its own PR, though.

Yup changes look pretty minimal. I'll open the PR once this gets merged.

BTW do you prefer for me to squash my commits when resolving PR comments so history ends up being a bit cleaner?

[davidhewitt]

BTW do you prefer for me to squash my commits when resolving PR comments so history ends up being a bit cleaner?

Yes please! Sometimes it makes sense to arrange things into multiple commits for future readers and that's fine, but when they're just "review feedback" then we like squashing 👍

(Because we occasionally keep multiple commits GH merge queue doesn't let us squash automatically. But maybe we should consider changing this, and instead of we want multiple commits open multiple PRs 🤔)

[davidhewitt]

Looks great, thanks again 👍

[snuderl]

BTW do you prefer for me to squash my commits when resolving PR comments so history ends up being a bit cleaner?

Yes please! Sometimes it makes sense to arrange things into multiple commits for future readers and that's fine, but when they're just "review feedback" then we like squashing 👍

(Because we occasionally keep multiple commits GH merge queue doesn't let us squash automatically. But maybe we should consider changing this, and instead of we want multiple commits open multiple PRs 🤔)

Squashed