Skip to content
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

bpo-40780: Fix failure of _Py_dg_dtoa to remove trailing zeros #20435

Merged
merged 5 commits into from
May 29, 2020
Merged

bpo-40780: Fix failure of _Py_dg_dtoa to remove trailing zeros #20435

merged 5 commits into from
May 29, 2020

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented May 26, 2020 •
edited
Loading

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

mdickinson
mdickinson commented May 26, 2020
View reviewed changes
Python/dtoa.c Outdated Show resolved Hide resolved
@mdickinson mdickinson marked this pull request as ready for review May 26, 2020 17:28
ericvsmith
ericvsmith reviewed May 26, 2020
View reviewed changes
Copy link
Member

@ericvsmith ericvsmith left a comment

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?

@mdickinson
Copy link
Member Author

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.

@mdickinson
Copy link
Member Author

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.)

@mdickinson
Copy link
Member Author

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;

@davidchambers
Copy link

Thank you for fixing this, @mdickinson. This experience has reminded me why I love open-source software development. ❤️

@mdickinson
Copy link
Member Author

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.

ericvsmith
ericvsmith approved these changes May 29, 2020
View reviewed changes
Copy link
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

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

LGTM.

@mdickinson mdickinson merged commit 895c9c1 into python:master May 29, 2020
@miss-islington
Copy link
Contributor

Thanks @mdickinson for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-20514 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label May 29, 2020
mdickinson added a commit that referenced this pull request May 29, 2020
@miss-islington @mdickinson
bpo-40780: Fix failure of _Py_dg_dtoa to remove trailing zeros (GH-20435
ad088ca 
) (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 <[email protected]>
@mdickinson mdickinson deleted the fix-g-mode-trailing-zero branch May 29, 2020 13:47
CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request May 30, 2020
@CuriousLearner
Merge branch 'master' of github.com:python/cpython into bpo-26543
d3d7100 
* '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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericvsmith ericvsmith ericvsmith approved these changes

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
@mdickinson @davidchambers @miss-islington @bedevere-bot @ericvsmith @the-knights-who-say-ni