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

mdickinson

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

mdickinson

@mdickinson

ericvsmith

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

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

@mdickinson

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

@mdickinson

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

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

@mdickinson

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

Choose a reason for hiding this comment

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

LGTM.

@miss-islington

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

@bedevere-bot

mdickinson added a commit that referenced this pull request

May 29, 2020

@miss-islington @mdickinson

) (GH-20514)

Co-authored-by: Mark Dickinson mdickinson@enthought.com

CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request

May 30, 2020

@CuriousLearner