Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-73487: Convert _decimal to use Argument Clinic (part 1)#137606
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
812708a to 439380eCompare This comment has been minimized.
This comment has been minimized.
AA-Turner commented Aug 10, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Is the
It looks worse than it actually is, only ~400 lines diff in |
skirpichev commented Aug 10, 2025
As I said, it's more or less a mechanical change. No changes in signatures, docstrings are copied from docstrings.h (though, AC enforces to have PEP summary line, most decimal docstrings don't follow this - that changed). There should be a minor speedup from using METH_FASTCALL (see issue thread for examples, I'll do benchmarks later, maybe add news). I'm planning to use also METH_METHOD where possible, but in a separate patch.
Different types, then method functions? I don't see how to make such split uniform. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
_decimal to use Argument ClinicCo-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
AA-Turner commented Aug 11, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Yes. I'm happy to look at the changes, just in a future PR instead of this one. |
This reverts commit 354d8db.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
_decimal to use Argument Clinic_decimal to use Argument Clinic (part 1)AA-Turner commented Aug 12, 2025
Now merged. Serhiy also suggested (in #137685 (comment)) that AC conversions don't need NEWS entries, perhaps remove from this PR? A |
AA-Turner commented Aug 12, 2025
|
skirpichev commented Aug 13, 2025
Thanks, I fixed errors. BTW, tool should display them all.
Removed. I fine with this, though PR affects performance and - thus - has user-visible changes. |
serhiy-storchaka commented Aug 13, 2025
If this affects performance, you can mention methods that have become significantly faster. |
Modules/_decimal/_decimal.c Outdated
| /*[clinic input] | ||
| _decimal.Decimal.to_integral_value | ||
| self as dec: self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not rename dec to self?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I did this, see reversion per reviewer request: 8b757da
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that in long term it is better to change few more lines, but get rid of the self converter unless it is absolutely necessary (if we need a behavior different from default). But this is just nitpicks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored.
skirpichev commented Aug 13, 2025
It's a long list, every function that previously used METH_VARARGS is affected. I can restore news again, does it make sense? |
serhiy-storchaka commented Aug 13, 2025
If it does not mention Argument Clinic and other implementation details. |
serhiy-storchaka left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, LGTM.
But since there were objections to this conversion from the author and a former developer, we need approval from several core developers who have worked on the decimal module for many years.
Modules/_decimal/_decimal.c Outdated
| /*[clinic input] | ||
| _decimal.Decimal.to_integral_value | ||
| self as dec: self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that in long term it is better to change few more lines, but get rid of the self converter unless it is absolutely necessary (if we need a behavior different from default). But this is just nitpicks.
skirpichev commented Aug 13, 2025
vstinner left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I like moving docstrings closer to function implementation, it helps to keep both consistent and up to date! The code is also easier to read. Hopefully, it may be even faster ;-)
picnixz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same opinion as Victor.
70730ad into python:mainUh oh!
There was an error while loading. Please reload this page.
vstinner commented Aug 13, 2025
Merged. Thank you! |
skirpichev commented Aug 13, 2025
Thanks for reviews! |
bedevere-bot commented Aug 13, 2025
|
…ython#137606) Co-authored-by: Adam Turner <9087854+aa-turner@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.