gh-73487: Convert ``_decimal`` to use Argument Clinic (part 1) by skirpichev · Pull Request #137606 · python/cpython · GitHub
Skip to content

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

Merged
vstinner merged 19 commits into
python:mainfrom
skirpichev:ac-decimal/73487
Aug 13, 2025
Merged

gh-73487: Convert _decimal to use Argument Clinic (part 1)#137606
vstinner merged 19 commits into
python:mainfrom
skirpichev:ac-decimal/73487

Conversation

@skirpichev

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

Copy link
Copy Markdown
Member

@bedevere-app bedevere-app Bot mentioned this pull request Aug 10, 2025
Closed
@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

AA-Turner commented Aug 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

@skirpichev

skirpichev commented Aug 10, 2025

Copy link
Copy Markdown
Member 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
skirpichev and others added 2 commits August 11, 2025 04:09
@skirpichev @AA-Turner
Apply suggestions from code review
2c53b48 
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@skirpichev
+1
354d8db
@AA-Turner

This comment was marked as resolved.

Sign in to view
AA-Turner
AA-Turner reviewed Aug 11, 2025
View reviewed changes
Comment thread Modules/_decimal/_decimal.c Outdated
@skirpichev

This comment was marked as resolved.

Sign in to view
@AA-Turner

AA-Turner commented Aug 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

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
@AA-Turner

AA-Turner commented Aug 12, 2025

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

AA-Turner commented Aug 12, 2025

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

skirpichev commented Aug 13, 2025

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

serhiy-storchaka commented Aug 13, 2025

Copy link
Copy Markdown
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
Comment thread Modules/_decimal/_decimal.c Outdated
/*[clinic input]
_decimal.Decimal.to_integral_value

self as dec: self

@serhiy-storchaka serhiy-storchaka Aug 13, 2025

Copy link
Copy Markdown
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?

@skirpichev skirpichev Aug 13, 2025

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

@serhiy-storchaka serhiy-storchaka Aug 13, 2025

Copy link
Copy Markdown
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 Aug 13, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Restored.

@skirpichev

skirpichev commented Aug 13, 2025

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

serhiy-storchaka commented Aug 13, 2025

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

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

Comment thread Modules/_decimal/_decimal.c Outdated
/*[clinic input]
_decimal.Decimal.to_integral_value

self as dec: self

@serhiy-storchaka serhiy-storchaka Aug 13, 2025

Copy link
Copy Markdown
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 picnixz and vstinner August 13, 2025 07:55
@skirpichev

skirpichev commented Aug 13, 2025

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

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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

@picnixz picnixz left a comment

Copy link
Copy Markdown
Member

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

vstinner commented Aug 13, 2025

Copy link
Copy Markdown
Member

Merged. Thank you!

@skirpichev
skirpichev deleted the ac-decimal/73487 branch August 13, 2025 13:10
@skirpichev

skirpichev commented Aug 13, 2025

Copy link
Copy Markdown
Member Author

Thanks for reviews!

@bedevere-bot

bedevere-bot commented Aug 13, 2025

Copy link
Copy Markdown

Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
@skirpichev @AA-Turner @Agent-Hellboy
pythongh-73487: Convert _decimal to use Argument Clinic (part 1) (p…
c654b9a 
…ython#137606)

Co-authored-by: Adam Turner <9087854+aa-turner@users.noreply.github.com>
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.

6 participants

@skirpichev @AA-Turner @serhiy-storchaka @vstinner @bedevere-bot @picnixz