Skip to content

gh-73487: Convert _decimal to use Argument Clinic (part 2) #137637

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

skirpichev
Copy link
Contributor

@skirpichev skirpichev commented Aug 11, 2025

@skirpichev

This comment was marked as outdated.

@skirpichev

This comment was marked as outdated.

@skirpichev skirpichev marked this pull request as ready for review August 13, 2025 15:26
@skirpichev skirpichev requested review from a team and erlend-aasland as code owners August 13, 2025 15:26
@AA-Turner
Copy link
Member

@skirpichev This PR is +7000/-2000, it's too big. Do you have any suggestions to split it up into parts?

A

@skirpichev
Copy link
Contributor Author

How about the first commit?

@vstinner
Copy link
Member

Same here. If you want me to review your PR, please split it into smaller PRs.

@skirpichev skirpichev force-pushed the ac-decimal/73487-pt2 branch from 6c8f61f to 1b7845e Compare August 13, 2025 17:07
@skirpichev
Copy link
Contributor Author

skirpichev commented Aug 13, 2025

Ok, I left only first commit. Is this still too much?

@vstinner
Copy link
Member

I would prefer a PR which would be 1/3 or 1/4 of that.

@skirpichev

This comment was marked as outdated.

@serhiy-storchaka
Copy link
Member

I would prefer a single PR containing all changes of the same kind than 10 different PRs.

Different kinds of changes can be presented in different PRs.

@skirpichev

This comment was marked as outdated.

@skirpichev

This comment was marked as outdated.

@skirpichev skirpichev marked this pull request as draft August 16, 2025 01:53
@skirpichev skirpichev force-pushed the ac-decimal/73487-pt2 branch from 1b7845e to c01448e Compare August 16, 2025 04:22
@skirpichev
Copy link
Contributor Author

Patch updated. Now it's roughly 1/3 of the previous version:

$ git diff --stat master..origin/ac-decimal/73487-pt2 Modules/_decimal/_decimal.c
 Modules/_decimal/_decimal.c | 400 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 320 insertions(+), 80 deletions(-)

I did no renaming of arguments. If this going to be in a dozen of prs - lets do this last. This pr includes all updates in interned strings (Include/ stuff). But in general split is more or less arbitrary.

Please review.

Documentation diff:

--- ref.txt 2025-08-16 07:41:45.006614985 +0300
+++ patch.txt   2025-08-16 07:22:18.425426268 +0300
@@ -291,14 +291,16 @@
      |      the context to the result.
      |
      |  power(self, /, a, b, modulo=None)
-     |      Compute a**b. If 'a' is negative, then 'b' must be integral. The result
-     |      will be inexact unless 'a' is integral and the result is finite and can
-     |      be expressed exactly in 'precision' digits.  In the Python version the
-     |      result is always correctly rounded, in the C version the result is almost
-     |      always correctly rounded.
+     |      Compute a**b.
      |
-     |      If modulo is given, compute (a**b) % modulo. The following restrictions
-     |      hold:
+     |      If 'a' is negative, then 'b' must be integral. The result will be
+     |      inexact unless 'a' is integral and the result is finite and can be
+     |      expressed exactly in 'precision' digits.  In the Python version the
+     |      result is always correctly rounded, in the C version the result is
+     |      almost always correctly rounded.
+     |
+     |      If modulo is given, compute (a**b) % modulo. The following
+     |      restrictions hold:
      |
      |          * all three arguments must be integral
      |          * 'b' must be nonnegative
@@ -473,10 +475,8 @@
      |  __floordiv__(self, value, /)
      |      Return self//value.
      |
-     |  __format__(...)
-     |      Default object formatter.
-     |
-     |      Return str(self) if format_spec is empty. Raise TypeError otherwise.
+     |  __format__(self, /, fmtarg, override=None)
+     |      Formats the Decimal according to fmtarg.
      |
      |  __ge__(self, value, /)
      |      Return self>=value.
@@ -651,13 +651,16 @@
      |      exactly.
      |
      |  exp(self, /, context=None)
-     |      Return the value of the (natural) exponential function e**x at the given
-     |      number.  The function always uses the ROUND_HALF_EVEN mode and the result
-     |      is correctly rounded.
+     |      Return the value of the (natural) exponential function e**x.
+     |
+     |      The function always uses the ROUND_HALF_EVEN mode and the result is
+     |      correctly rounded.
      |
      |  fma(self, /, other, third, context=None)
-     |      Fused multiply-add.  Return self*other+third with no rounding of the
-     |      intermediate product self*other.
+     |      Fused multiply-add.
+     |
+     |      Return self*other+third with no rounding of the intermediate product
+     |      self*other.
      |
      |          >>> Decimal(2).fma(3, 5)
      |          Decimal('11')
@@ -704,18 +707,23 @@
      |      otherwise.
      |
      |  ln(self, /, context=None)
-     |      Return the natural (base e) logarithm of the operand. The function always
-     |      uses the ROUND_HALF_EVEN mode and the result is correctly rounded.
+     |      Return the natural (base e) logarithm of the operand.
+     |
+     |      The function always uses the ROUND_HALF_EVEN mode and the result is
+     |      correctly rounded.
      |
      |  log10(self, /, context=None)
-     |      Return the base ten logarithm of the operand. The function always uses the
-     |      ROUND_HALF_EVEN mode and the result is correctly rounded.
+     |      Return the base ten logarithm of the operand.
+     |
+     |      The function always uses the ROUND_HALF_EVEN mode and the result is
+     |      correctly rounded.
      |
      |  logb(self, /, context=None)
-     |      For a non-zero number, return the adjusted exponent of the operand as a
-     |      Decimal instance.  If the operand is a zero, then Decimal('-Infinity') is
-     |      returned and the DivisionByZero condition is raised. If the operand is
-     |      an infinity then Decimal('Infinity') is returned.
+     |      Return the adjusted exponent of the operand as a Decimal instance.
+     |
+     |      If the operand is a zero, then Decimal('-Infinity') is returned and the
+     |      DivisionByZero condition is raised. If the operand is an infinity then
+     |      Decimal('Infinity') is returned.
      |
      |  logical_and(self, /, other, context=None)
      |      Return the digit-wise 'and' of the two (logical) operands.
@@ -746,14 +754,10 @@
      |      values of the operands.
      |
      |  next_minus(self, /, context=None)
-     |      Return the largest number representable in the given context (or in the
-     |      current default context if no context is given) that is smaller than the
-     |      given operand.
+     |      Returns the largest representable number smaller than itself.
      |
      |  next_plus(self, /, context=None)
-     |      Return the smallest number representable in the given context (or in the
-     |      current default context if no context is given) that is larger than the
-     |      given operand.
+     |      Returns the smallest representable number larger than itself.
      |
      |  next_toward(self, /, other, context=None)
      |      If the two operands are unequal, return the number closest to the first
@@ -762,11 +766,12 @@
      |      to be the same as the sign of the second operand.
      |
      |  normalize(self, /, context=None)
-     |      Normalize the number by stripping the rightmost trailing zeros and
-     |      converting any result equal to Decimal('0') to Decimal('0e0').  Used
-     |      for producing canonical values for members of an equivalence class.
-     |      For example, Decimal('32.100') and Decimal('0.321000e+2') both normalize
-     |      to the equivalent value Decimal('32.1').
+     |      Normalize the number by stripping trailing 0s
+     |
+     |      This also change anything equal to 0 to 0e0.  Used for producing
+     |      canonical values for members of an equivalence class.  For example,
+     |      Decimal('32.100') and Decimal('0.321000e+2') both normalize to
+     |      the equivalent value Decimal('32.1').
      |
      |  number_class(self, /, context=None)
      |      Return a string describing the class of the operand.
@@ -860,8 +865,9 @@
      |      of the first operand are unchanged.
      |
      |  sqrt(self, /, context=None)
-     |      Return the square root of the argument to full precision. The result is
-     |      correctly rounded using the ROUND_HALF_EVEN rounding mode.
+     |      Return the square root of the argument to full precision.
+     |
+     |      The result is correctly rounded using the ROUND_HALF_EVEN rounding mode.
      |
      |  to_eng_string(self, /, context=None)
      |      Convert to an engineering-type string.
@@ -1762,17 +1768,20 @@

 FUNCTIONS
     IEEEContext(bits, /)
-        Return a context object initialized to the proper values for one of the
-        IEEE interchange formats.  The argument must be a multiple of 32 and less
-        than IEEE_CONTEXT_MAX_BITS.
+        Return a context, initialized as one of the IEEE interchange formats.
+
+        The argument must be a multiple of 32 and less than
+        IEEE_CONTEXT_MAX_BITS.

     getcontext()
         Get the current default context.

     localcontext(ctx=None, **kwargs)
-        Return a context manager that will set the default context to a copy of ctx
-        on entry to the with-statement and restore the previous default context when
-        exiting the with-statement. If no context is specified, a copy of the current
+        Return a context manager for a copy of the supplied context.
+
+        That will set the default context to a copy of ctx on entry to the
+        with-statement and restore the previous default context when exiting
+        the with-statement. If no context is specified, a copy of the current
         default context is used.

     setcontext(context, /)

@skirpichev skirpichev marked this pull request as ready for review August 16, 2025 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants