8000 gh-132097: remove unnecessary clinic casts to `PyCFunction` and others by picnixz · Pull Request #131665 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-132097: remove unnecessary clinic casts to PyCFunction and others #131665

New issue 10000

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
Closed
wants to merge 12 commits into from
Closed

gh-132097: remove unnecessary clinic casts to PyCFunction and others #131665

wants to merge 12 commits into from

Conversation

picnixz
Copy link
Member
@picnixz picnixz commented Mar 24, 2025 •
edited
Loading

@bedevere-app bedevere-app bot mentioned this pull request Mar 24, 2025
Closed
@picnixz
Copy link
Member Author
picnixz commented Apr 4, 2025

@vstinner This one is purely cosmetic but it affects lots of files. It's about removing redundant casts. I've removed some redundant casts with non-clinic files but this one just removes the casts generated by clinic.

I'm not entirely sure I want to merge this for the following reason:

  • let's say clinic incorrectly generates a signature, and the signature then becomes incompatible. With this change, this would affect the PR with a bad clinic signature. However, this means that we need to fix clinic in the meantime.
  • Without removing the explicit casts, we can at least make other PRs relying on clinic not affected by subtle UBs that may or may not be impactful.
  • For WASM, UBs are strictly checked: "Bad fpcasts are an issue in WebAssembly. WASM's indirect_call has strict function signature checks. Argument count, types, and return type must match." (this is a comment for _PyCFunction_TrampolineCall in pycore_object.h (which is also duplicated in pycore_emscripten_trampoline.h).
  • So technically, while it's a UB for all platforms, it's much more annoying on Emscripten. So maybe it's better that UBs are always detected at compile time.

I'm going to first address the fastcall issue before continuing here so take your time. I won't commit if you're not happy with this change or if you prefer that we hold it until we see a real issue.

@picnixz picnixz changed the title gh-111178: remove unnecessary clinic casts to PyCFunction gh-111178: remove unnecessary clinic casts to PyCFunction and others Apr 5, 2025
@picnixz picnixz changed the title gh-111178: remove unnecessary clinic casts to PyCFunction and others gh-131665: remove unnecessary clinic casts to PyCFunction and others Apr 5, 2025
@picnixz picnixz changed the title gh-131665: remove unnecessary clinic casts to PyCFunction and others gh-132097: remove unnecessary clinic casts to PyCFunction and others Apr 5, 2025
@bedevere-app bedevere-app bot mentioned this pull request Apr 5, 2025
Closed
@picnixz
Copy link
Member Author
picnixz commented Apr 20, 2025

I more or less added explicit casts for WASM in #132406 so what's left here is just removing redundant casts. If someone wants to push for that, we'll do it but it will create conflicts in many generated files (which, thankfully, are easy to fix with make clinic). That being said, it's not that useful for now. If I were to be alone on the project, I would have pushed this one. But I'm not and I eventually don't see a real value that this PR brings. So I will close this.

@picnixz picnixz closed this Apr 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 will be requested when the pull request is marked ready for review kumaraditya303 is a code owner

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland will be requested when the pull request is marked ready for review erlend-aasland is a code owner

@corona10 corona10 Awaiting requested review from corona10 corona10 will be requested when the pull request is marked ready for review corona10 is a code owner

@FFY00 FFY00 Awaiting requested review from FFY00 FFY00 will be requested when the pull request is marked ready for review FFY00 is a code owner

@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra JelleZijlstra will be requested when the pull request is marked ready for review JelleZijlstra is a code owner

@AlexWaygood AlexWaygood Awaiting requested review from AlexWaygood AlexWaygood will be requested when the pull request is marked ready for review AlexWaygood is a code owner

@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger will be requested when the pull request is marked ready for review rhettinger is a code owner

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag berkerpeksag will be requested when the pull request is marked ready for review berkerpeksag is a code owner

@gpshead gpshead Awaiting requested review from gpshead gpshead will be requested when the pull request is marked ready for review gpshead is a code owner

@pganssle pganssle Awaiting requested review from pganssle pganssle will be requested when the pull request is marked ready for review pganssle is a code owner

@abalkin abalkin Awaiting requested review from abalkin abalkin will be requested when the pull request is marked ready for review abalkin is a code owner

@brettcannon brettcannon Awaiting requested review from brettcannon brettcannon will be requested when the pull request is marked ready for review brettcannon is a code owner

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently will be requested when the pull request is marked ready for review ericsnowcurrently is a code owner

@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan will be requested when the pull request is marked ready for review ncoghlan is a code owner

@warsaw warsaw Awaiting requested review from warsaw warsaw will be requested when the pull request is marked ready for review warsaw is a code owner

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka serhiy-storchaka will be requested when the pull request is marked ready for review serhiy-storchaka is a code owner

@1st1 1st1 Awaiting requested review from 1st1 1st1 will be requested when the pull request is marked ready for review 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov will be requested when the pull request is marked ready for review asvetlov is a code owner

@willingc willingc Awaiting requested review from willingc willingc will be requested when the pull request is marked ready for review willingc is a code owner

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@picnixz
0