Skip to content

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.

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
Merged
merged 19 commits into from
Aug 13, 2025
Merged

gh-73487: Convert _decimal to use Argument Clinic (part 1) #137606

merged 19 commits into from
Aug 13, 2025
+1,205 −276

Conversation

skirpichev
Copy link
Contributor

@skirpichev skirpichev commented Aug 10, 2025 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Aug 10, 2025
Open
@skirpichev skirpichev changed the title gh-73487: convert the decimal module to use AC gh-73487: convert the _decimal module to use AC Aug 10, 2025
@skirpichev

This comment has been minimized.

Sign in to view
@AA-Turner
Copy link
Member

AA-Turner commented Aug 10, 2025 •
edited
Loading

Is the help(decimal) output identical before and after the PR?

I appreciate suggestion on how to make it easier to review.

It looks worse than it actually is, only ~400 lines diff in _decimal.c and ~190 lines of docstring-only changes in _decimal/docstrings.h. You could maybe split up the Decimal methods into a few groups to reduce the size of the PR, but no obvious such groupings spring to mind.

@skirpichev
Copy link
Contributor Author

It looks worse than it actually is

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.

no obvious such groupings spring to mind.

Different types, then method functions? I don't see how to make such split uniform.

@AA-Turner

This comment was marked as outdated.

Sign in to view
AA-Turner

This comment was marked as outdated.

Sign in to view

@AA-Turner AA-Turner changed the title gh-73487: convert the _decimal module to use AC gh-73487: Convert _decimal to use Argument Clinic Aug 10, 2025
@AA-Turner

This comment was marked as resolved.

Sign in to view
AA-Turner
AA-Turner reviewed Aug 11, 2025
View reviewed changes
@skirpichev

This comment was marked as resolved.

Sign in to view
@AA-Turner
Copy link
Member

AA-Turner commented Aug 11, 2025 •
edited
Loading

Do you think it's now too hard to review?

Yes. I'm happy to look at the changes, just in a future PR instead of this one.

@skirpichev
Revert "+1"
81cba59 
This reverts commit 354d8db.
@skirpichev skirpichev requested a review from AA-Turner August 11, 2025 02:08
@AA-Turner

This comment was marked as resolved.

Sign in to view
AA-Turner

This comment was marked as resolved.

Sign in to view

@skirpichev skirpichev changed the title gh-73487: Convert _decimal to use Argument Clinic gh-73487: Convert _decimal to use Argument Clinic (part 1) Aug 11, 2025
@skirpichev
Copy link
Contributor Author

I would appreciate if #137624 can be merged first. It seems, Serhiy has only few nitpicks on that pr.

FYI: I cleaned some comments as redundant.

@rhettinger rhettinger removed their request for review August 12, 2025 16:31
@AA-Turner
Copy link
Member

I would appreciate if #137624 can be merged first.

Now merged.

Serhiy also suggested (in #137685 (comment)) that AC conversions don't need NEWS entries, perhaps remove from this PR?

A

@AA-Turner
Copy link
Member 

Error in file 'Modules/_decimal/_decimal.c' on line 4616:
Warning:
Docstring lines for '_decimal.Decimal.to_integral_value' are too long!
Lines should be no longer than 72 characters.

Warning:
Docstring lines for '_decimal.Decimal.to_integral' are too long!
Lines should be no longer than 72 characters.

Warning:
Docstring lines for '_decimal.Decimal.to_integral_exact' are too long!
Lines should be no longer than 72 characters.

Summary line for '_decimal.Decimal.adjusted' is too long!
The summary line must be no longer than 72 characters.

@skirpichev
Copy link
Contributor Author

Now merged.

Thanks, I fixed errors. BTW, tool should display them all.

Serhiy also suggested (in #137685 (comment)) that AC conversions don't need NEWS entries, perhaps remove from this PR?

Removed. I fine with this, though PR affects performance and - thus - has user-visible changes.

@serhiy-storchaka
Copy link
Member

If this affects performance, you can mention methods that have become significantly faster.

serhiy-storchaka
serhiy-storchaka reviewed Aug 13, 2025
View reviewed changes
Modules/_decimal/_decimal.c Outdated
/*[clinic input]
_decimal.Decimal.to_integral_value
self as dec: self
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restored.

@skirpichev
Copy link
Contributor Author

If this affects performance, you can mention methods that have become significantly faster.

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
Copy link
Member

I can restore news again, does it make sense?

If it does not mention Argument Clinic and other implementation details.

serhiy-storchaka
serhiy-storchaka approved these changes Aug 13, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a 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
Copy link
Member

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 skirpichev requested review from vstinner and picnixz August 13, 2025 07:55
@skirpichev
Copy link
Contributor Author

Ok, I asked @vstinner and @picnixz for review.

Raymond removed request for review, I hope this doesn't mean he is -1 on this.

vstinner
vstinner approved these changes Aug 13, 2025
View reviewed changes
Copy link
Member

@vstinner vstinner left a 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
picnixz approved these changes Aug 13, 2025
View reviewed changes
Copy link
Member

@picnixz picnixz left a 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.

@vstinner vstinner merged commit 70730ad into python:main Aug 13, 2025
46 checks passed
@vstinner
Copy link
Member

Merged. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@picnixz picnixz picnixz approved these changes

@AA-Turner AA-Turner Awaiting requested review from AA-Turner

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

Successfully merging this pull request may close these issues.
5 participants
@skirpichev @AA-Turner @serhiy-storchaka @vstinner @picnixz