gh-114563: C decimal falls back to pydecimal for unsupported format strings by belm0 · Pull Request #114879 · python/cpython

belm0 · 2024-02-01T20:44:37Z

When the mpdecimal lib fails to parse a format string, we fall back to the pydecimal format implementation.

Notably this allows complex workarounds for 'z' format support to be removed. (mpdecimal added 'z' support in version 4.0.0, so this becomes efficient in the long term.)

remove 'z' format implementation from C decimal
fix 'z' format memory leak
fix 'z' format applied to type 'F'
fix missing '#' format support

Suggested and prototyped by Stefan Krah.

Fixesgh-114563, gh-91060

…rmat strings immediate merits: * eliminate complex workarounds for 'z' format support (NOTE: mpdecimal recently added 'z' support, so this becomes efficient in the long term.) * fix 'z' format memory leak * fix 'z' format applied to 'F' * fix missing '#' format support Suggested and prototyped by Stefan Krah. Fixespythongh-114563, pythongh-91060

mdickinson

LGTM, with one suggestion about the exception handling for the __format__ call.

@skrah Thank you for your help with this. If you happen to have the bandwidth and inclination to take a quick glance at this PR, I'd very much appreciate your input.

skrah · 2024-02-10T14:50:09Z

If I checkout d96a8cd and then apply my own patches, I reach the exact same state as in this PR.

When comparing the result to 3.9, the diff in the dec_format area is clean and manageable.

I tested my own patches with deccheck (z-format extended version), so I approve!

@serhiy-storchaka's pydec_format changes look correct to me, but I'd have to look up some of the new functions, so I'll stay out of that part.

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

serhiy-storchaka

LGTM.

Is it okay that the PyDecimal instance can have a different context?

skrah · 2024-02-10T20:11:39Z

Is it okay that the PyDecimal instance can have a different context?

This is an excellent observation! It is the one thing that cannot be caught by deccheck, because the _decimal and _pydecimal contexts are always in sync. _pydecimal.__format__ needs rounding and capitals.

Maybe the easiest thing would be to add another private method to _pydecimal, something like:

def fallback_format(self, specifier, rounding, caps):
# make a new context with rounding and caps
# return __format__ with that context

skrah · 2024-02-11T17:53:15Z

This is a clever use of the _decimal context! It works because ctx.rounding is a string and ctx.capitals is a bool. For others who happen to come by this issue, I want to (pedantically) point out that the _pydecimal and _decimal contexts are not in general interchangeable.

LGTM.

serhiy-storchaka

Now please add a NEWS entry (it is a user visible change), and it will be done.

Describe what behavior was changed, not implementation details.

miss-islington-app · 2024-02-12T11:18:16Z

Thanks @belm0 for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington-app · 2024-02-12T11:18:16Z

Thanks @belm0 for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington-app · 2024-02-12T11:18:28Z

Sorry, @belm0 and @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 72340d15cdfdfa4796fdd7c702094c852c2b32d2 3.12

miss-islington-app · 2024-02-12T11:18:44Z

Sorry, @belm0 and @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 72340d15cdfdfa4796fdd7c702094c852c2b32d2 3.11

serhiy-storchaka · 2024-02-12T11:25:09Z

@belm0, do you mind to create a backport for 3.12?

It will be easier to backport then from 3.12 to 3.11 than directly to 3.11.

bedevere-app · 2024-02-12T16:40:13Z

GH-115353 is a backport of this pull request to the 3.12 branch.

…ormat strings (GH-114879) (GH-115353) Immediate merits: * eliminate complex workarounds for 'z' format support (NOTE: mpdecimal recently added 'z' support, so this becomes efficient in the long term.) * fix 'z' format memory leak * fix 'z' format applied to 'F' * fix missing '#' format support Suggested and prototyped by Stefan Krah. Fixesgh-114563, gh-91060 (cherry picked from commit 72340d1) Co-authored-by: John Belmonte <john@neggie.net> Co-authored-by: Stefan Krah <skrah@bytereef.org>

…unsupported format strings (pythonGH-114879) (pythonGH-115353) Immediate merits: * eliminate complex workarounds for 'z' format support (NOTE: mpdecimal recently added 'z' support, so this becomes efficient in the long term.) * fix 'z' format memory leak * fix 'z' format applied to 'F' * fix missing 'GH-' format support Suggested and prototyped by Stefan Krah. Fixespythongh-114563, pythongh-91060 (cherry picked from commit 72340d1) (cherry picked from commit 09c98e4) Co-authored-by: John Belmonte <john@neggie.net> Co-authored-by: Stefan Krah <skrah@bytereef.org>

bedevere-app · 2024-02-13T04:20:52Z

GH-115384 is a backport of this pull request to the 3.11 branch.

…ormat strings (GH-114879) (GH-115384) Immediate merits: * eliminate complex workarounds for 'z' format support (NOTE: mpdecimal recently added 'z' support, so this becomes efficient in the long term.) * fix 'z' format memory leak * fix 'z' format applied to 'F' * fix missing 'GH-' format support Suggested and prototyped by Stefan Krah. Fixesgh-114563, gh-91060 (cherry picked from commit 72340d1) (cherry picked from commit 09c98e4) Co-authored-by: Stefan Krah <skrah@bytereef.org>

…rmat strings (pythonGH-114879) Immediate merits: * eliminate complex workarounds for 'z' format support (NOTE: mpdecimal recently added 'z' support, so this becomes efficient in the long term.) * fix 'z' format memory leak * fix 'z' format applied to 'F' * fix missing '#' format support Suggested and prototyped by Stefan Krah. Fixespythongh-114563, pythongh-91060 Co-authored-by: Stefan Krah <skrah@bytereef.org>

belm0 added 3 commits February 1, 2024 12:26

add decimal tests
d49d679

revert 'z' format support in C decimal
b526004

belm0 requested a review from rhettinger as a code owner February 1, 2024 20:44

bedevere-appBot added the awaiting review label Feb 1, 2024

bedevere-appBot mentioned this pull request Feb 1, 2024
Possible memory leak in _cdecimal.c with 'z' format #114563
Closed

erlend-aasland requested a review from mdickinson February 1, 2024 21:52

rhettinger removed their request for review February 1, 2024 23:45

mdickinson reviewed Feb 10, 2024
View reviewed changes

Comment threadModules/_decimal/_decimal.c Outdated

mdickinson reviewed Feb 10, 2024
View reviewed changes

serhiy-storchaka reviewed Feb 10, 2024
View reviewed changes

Comment threadModules/_decimal/_decimal.c Outdated
Comment threadModules/_decimal/_decimal.c Outdated
Comment threadModules/_decimal/_decimal.c Outdated
Comment threadModules/_decimal/_decimal.c
Comment threadLib/test/test_decimal.py

Apply suggestions from code review
d37b5b8
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

serhiy-storchaka reviewed Feb 10, 2024
View reviewed changes

Comment threadLib/test/test_decimal.py Outdated

belm0 added 3 commits February 10, 2024 10:05

disable failing null fill test
d2b6893

only override message string on ValueError
feb7469

corrected null fill char test
5803d9c

serhiy-storchaka approved these changes Feb 10, 2024
View reviewed changes

bedevere-appBot added awaiting merge and removed awaiting review labels Feb 10, 2024

belm0 commented Feb 10, 2024
View reviewed changes

Comment threadModules/_decimal/_decimal.c

belm0 added 2 commits February 10, 2024 16:26

honor round and capitals context in fallback
dcee58f

format fallback context tests
7a2ba4b

serhiy-storchaka reviewed Feb 11, 2024
View reviewed changes

Comment threadModules/_decimal/_decimal.c Outdated

pass context directly to __format__()
ab4f2d7

serhiy-storchaka approved these changes Feb 11, 2024
View reviewed changes

skrah approved these changes Feb 11, 2024
View reviewed changes

serhiy-storchaka reviewed Feb 11, 2024
View reviewed changes

📜🤖 Added by blurb_it.
3b1b8f4

serhiy-storchaka reviewed Feb 11, 2024
View reviewed changes

Comment threadMisc/NEWS.d/next/Library/2024-02-11-20-23-36.gh-issue-114563.RzxNYT.rst Outdated

fix NEWS formatting
06e371c

serhiy-storchaka approved these changes Feb 11, 2024
View reviewed changes

serhiy-storchaka merged commit 72340d1 into python:mainFeb 12, 2024

bedevere-appBot removed the awaiting merge label Feb 12, 2024

serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Feb 12, 2024

miss-islington-appBot assigned serhiy-storchaka Feb 12, 2024

serhiy-storchaka removed the needs backport to 3.11 only security fixes label Feb 12, 2024

belm0 deleted the fix-issue-114563 branch February 12, 2024 15:06

belm0 mentioned this pull request Feb 12, 2024
Python Decimal supports '#' format, C Decimal does not. #91060
Closed

bedevere-appBot removed the needs backport to 3.12 only security fixes label Feb 12, 2024

serhiy-storchaka mentioned this pull request Feb 12, 2024
Make # replacing more strict python/cherry-picker#114
Closed

serhiy-storchaka removed their assignment Jul 15, 2026

Uh oh!

Uh oh!

Conversation

belm0 commented Feb 1, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mdickinson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skrah commented Feb 10, 2024

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

skrah commented Feb 10, 2024

Uh oh!

Uh oh!

Uh oh!

skrah commented Feb 11, 2024

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miss-islington-appBot commented Feb 12, 2024

Uh oh!

miss-islington-appBot commented Feb 12, 2024

Uh oh!

miss-islington-appBot commented Feb 12, 2024

Uh oh!

miss-islington-appBot commented Feb 12, 2024

Uh oh!

serhiy-storchaka commented Feb 12, 2024

Uh oh!

bedevere-appBot commented Feb 12, 2024

Uh oh!

bedevere-appBot commented Feb 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

belm0 commented Feb 1, 2024•
edited
Loading

mdickinson left a comment •
edited
Loading