Uh oh!
There was an error while loading. Please reload this page.
gh-73487: Convert _decimal to use Argument Clinic (part 1) - #137606
Conversation
812708a to
439380eCompare
This comment has been minimized.
This comment has been minimized.
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.
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. |
| /*[clinic input] | ||
| _decimal.Decimal.to_integral_value | ||
| self as dec: self |
There was a problem hiding this comment.
Why not rename dec to self?
There was a problem hiding this comment.
Initially I did this, see reversion per reviewer request: 8b757da
There was a problem hiding this comment.
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
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.
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.
| /*[clinic input] | ||
| _decimal.Decimal.to_integral_value | ||
| self as dec: self |
There was a problem hiding this comment.
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.
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 ;-)
Uh 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.