GeezerVison is showing github.com — gh-132097: remove unnecessary clinic casts to `PyCFunction` and others by picnixz · Pull Request #131665 · python/cpython
Conversation
picnixz
commented
Mar 24, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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
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
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
|
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
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
AA-Turner
Awaiting requested review from AA-Turner
AA-Turner will be requested when the pull request is marked ready for review
AA-Turner is a code owner
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|