bpo-40780: Fix failure of _Py_dg_dtoa to remove trailing zeros by mdickinson · Pull Request #20435 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits5 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
The dtoa.c
code underlying string formatting wasn't stripping trailing zeros in some cases where it should have been. This PR fixes that.
Making this a draft PR for now; I still need to add regression tests. EDIT: tests added
https://bugs.python.org/issue40780
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tracking this down, @mdickinson. It's hard to believe that after all these years a bug like this would still exist.
This change looks good to me.
Is it worth adding this to the comment at line 30? Or is this too trivial to matter?
Also, if this really is a bug in Gay's code, should we report it upstream? Or has too much water passed under the bridge for it to matter any more?
Is it worth adding this to the comment at line 30?
Yes, I'll do that. Reporting upstream seems like the right thing to do - I'll do that, too.
I changed the backport labels: as discussed on the issue, let's not backport to 3.7 or 3.8.
I also added a comment about the change to the top of the dtoa.c file, and reworked the logic so that it's clearer that the zero-stripping is safe.
Note that it's fine to return an empty string, and that's mostly what dtoa.c already does when the formatted output will be zero. (The exception is on an exact zero, where the string "0"
is returned.)
Reporting upstream seems like the right thing to do - I'll do that, too.
The upstream code has diverged, and this case is already fixed upstream, using essentially the same code that I ended up using here (but in a different place). So I won't report upstream.
I'm looking at http://www.netlib.org/fp/dtoa.c directly; I can't find any sort of changelog or other information about what changed and when, unfortunately. There may be other upstream fixes that we should be looking at applying, but I don't see a way to find out what those fixes are.
The relevant code in the current snapshot of dtoa.c is at line 6163, and looks like this:
retc:
while(s > buf && s[-1] == '0')
--s;
Thank you for fixing this, @mdickinson. This experience has reminded me why I love open-source software development. ❤️
Hi @ericvsmith. I made a few changes; would you have a moment to take a second look?
Sorry for the delay here. I got bogged down trying to understand more of the way that _Py_dg_dtoa
works and before I knew it I was halfway towards a rewrite of the whole function. But I think it's better to get this small non-invasive fix in soon.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Thanks @mdickinson for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖
mdickinson added a commit that referenced this pull request
) (GH-20514)
Fix failure of _Py_dg_dtoa to remove trailing zeros
Add regression test and news entry
Add explanation about why it's safe to strip trailing zeros
Make code safer, clean up comments, add change note at top of file
Nitpick: avoid implicit int-to-float conversion in tests (cherry picked from commit 895c9c1)
Co-authored-by: Mark Dickinson mdickinson@enthought.com
CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request
- 'master' of github.com:python/cpython: (497 commits) bpo-40061: Fix a possible refleak in _asynciomodule.c (pythonGH-19748) bpo-40798: Generate a different message for already removed elements (pythonGH-20483) closes bpo-29017: Update the bindings for Qt information with PySide2 (pythonGH-20149) bpo-39885: Make IDLE context menu cut and copy work again (pythonGH-18951) bpo-29882: Add an efficient popcount method for integers (python#771) Further de-linting of zoneinfo module (python#20499) bpo-40780: Fix failure of _Py_dg_dtoa to remove trailing zeros (pythonGH-20435) Indicate that abs() method accept argument that implement abs(), just like call() method in the docs (pythonGH-20509) bpo-39040: Fix parsing of email mime headers with whitespace between encoded-words. (pythongh-17620) bpo-40784: Fix sqlite3 deterministic test (pythonGH-20448) bpo-30064: Properly skip unstable loop.sock_connect() racing test (pythonGH-20494) Note the output ordering of combinatoric functions (pythonGH-19732) bpo-40474: Updated coverage.yml to better report coverage stats (python#19851) bpo-40806: Clarify that itertools.product immediately consumes its inpt (pythonGH-20492) bpo-1294959: Try to clarify the meaning of platlibdir (pythonGH-20332) bpo-37878: PyThreadState_DeleteCurrent() was not removed (pythonGH-20489) bpo-40777: Initialize PyDateTime_IsoCalendarDateType.tp_base at run-time (pythonGH-20493) bpo-40755: Add missing multiset operations to Counter() (pythonGH-20339) bpo-25920: Remove socket.getaddrinfo() lock on macOS (pythonGH-20177) bpo-40275: Fix test.support.threading_helper (pythonGH-20488) ...