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

refactor to remove add_to_module functions from generated code #4009

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Mar 29, 2024

Related to #3989

@alex / @reaperhulk, want to try this branch with the cryptography CI and see if it fixes (or reduces) the problem?

This reshapes the generated macro output so that it only emits constants (no functions) and then we leave the implementations inside PyO3's impl_ module, as I suggested on that issue. This is probably better for binary size / compile times even if it doesn't help the coverage problem.

Copy link

codspeed-hq bot commented Mar 29, 2024

CodSpeed Performance Report

Merging #4009 will not alter performance

Comparing davidhewitt:add-to-module (9de7016) with main (dd17102)

🎉 Hooray! codspeed-rust just leveled up to 2.4.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

✅ 68 untouched benchmarks

@alex
Copy link
Contributor

alex commented Mar 29, 2024

This significantly improved the coverage situation. We still have a few things missing coverage:

  • All invocations of pyo3::import_exception
  • Exactly one pyclass
  • One other bizzaro line (that's probably not related to pyo3)

@alex
Copy link
Contributor

alex commented Mar 29, 2024

For clarity, I think it makes sense to merge this and then we can do follow up PRs resolving the other coverage issues.

@davidhewitt
Copy link
Member Author

Great, will merge now!

@davidhewitt davidhewitt enabled auto-merge March 29, 2024 11:53
@davidhewitt davidhewitt added this pull request to the merge queue Mar 29, 2024
Merged via the queue into PyO3:main with commit cf74624 Mar 29, 2024
42 of 44 checks passed
@davidhewitt davidhewitt deleted the add-to-module branch March 29, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants