[issue1580] Use shorter float repr when possible

[Nick Coghlan, 2009-04-09 10:52:04]

I didn't actually dig into the actual number crunching code all that much - that
is something the test suite will cover pretty well, and it would also take more
time than I'm willing to spend on it :)

Main concerns are the lack of any indication in the pystrtod header file that
callers of PyOS_double_to_string are expected to free the returned buffer, and
an apparent memory leak in complexobject.c.

Other comments about things that just seem unnecessary rather than wrong.

http://codereview.appspot.com/33084/diff/1001/53
File Include/pystrtod.h (right):

http://codereview.appspot.com/33084/diff/1001/53#newcode12
Line 12: PyAPI_FUNC(char *) PyOS_double_to_string(double val,
Looking at the way this is used, I suggest a comment here indicating that the
caller is responsible for invoking PyMem_Free on the return value from this
function.

http://codereview.appspot.com/33084/diff/1001/62
File Lib/test/test_float.py (right):

http://codereview.appspot.com/33084/diff/1001/62#newcode14
Line 14: #locate file with float format test values
The following if statement seems redundant. When test_float.py is run directly
as the main module, __file__ and sys.argv[0] should be the same thing (the only
time those two values will be different inside __main__ is when the file being
executed is actually a __main__.py file inside a directory, zipfile or package)

http://codereview.appspot.com/33084/diff/1001/63
File Lib/test/test_types.py (right):

http://codereview.appspot.com/33084/diff/1001/63#newcode471
Line 471: #  after the py3k-short-float-repr merge
Is there a reason this test can't be put back in before the merge back to the
main py3k branch?

http://codereview.appspot.com/33084/diff/1001/55
File Objects/complexobject.c (right):

http://codereview.appspot.com/33084/diff/1001/55#newcode413
Line 413: buf = PyMem_Malloc(len);
Where does buf get freed?

http://codereview.appspot.com/33084/diff/1001/45
File Python/ast.c (right):

http://codereview.appspot.com/33084/diff/1001/45#newcode3165
Line 3165: PyFPE_START_PROTECT("strtod", return 0)
Why change these from the atof form to the strtod-with-NULL form? Just a
micro-optimisation to get rid of the redundant C function call?

http://codereview.appspot.com/33084/diff/1001/47
File Python/marshal.c (right):

http://codereview.appspot.com/33084/diff/1001/47#newcode669
Line 669: PyFPE_START_PROTECT("strtod", break)
Again with the change of an atof call to a direct strtod call. As with ast.c,
not wrong but also not really relevant to the current task.

[ericvsmith, 2009-04-09 11:44:20]

Thanks for the comments, Nick. I've answered the few that apply to me, Mark can
answer the others when he's back in town.

http://codereview.appspot.com/33084/diff/1001/63
File Lib/test/test_types.py (right):

http://codereview.appspot.com/33084/diff/1001/63#newcode471
Line 471: #  after the py3k-short-float-repr merge
On 2009/04/09 10:52:04, Nick Coghlan wrote:
> Is there a reason this test can't be put back in before the merge back to the
> main py3k branch?

It's just a matter of timing. I'm very close to having it working again. It will
either happen when we merge back, or within a few days.

http://codereview.appspot.com/33084/diff/1001/55
File Objects/complexobject.c (right):

http://codereview.appspot.com/33084/diff/1001/55#newcode413
Line 413: buf = PyMem_Malloc(len);
On 2009/04/09 10:52:04, Nick Coghlan wrote:
> Where does buf get freed?

Good catch. I'll fix that.

[ericvsmith, 2009-04-09 11:49:00]

Also a good idea. I'll check both of these in momentarily, once the tests pass.

http://codereview.appspot.com/33084/diff/1001/53
File Include/pystrtod.h (right):

http://codereview.appspot.com/33084/diff/1001/53#newcode12
Line 12: PyAPI_FUNC(char *) PyOS_double_to_string(double val,
I'll add that.

[ericvsmith, 2009-04-09 12:50:33]

http://codereview.appspot.com/33084/diff/1001/47
File Python/marshal.c (right):

http://codereview.appspot.com/33084/diff/1001/47#newcode241
Line 241: if (!buf)
A known post-merge task is to fix up w_object to handle these allocation
failures better. It's complicated by the fact that w_object is recursive. I'd
like to use p->error to store an "out of memory" error state, but I need to comb
through the code and make sure it's handled correctly. I'll probably post
another Rietveld patch when I get around to fixing it.

[dickinsm, 2009-04-12 10:03:25]

http://codereview.appspot.com/33084/diff/1001/62
File Lib/test/test_float.py (right):

http://codereview.appspot.com/33084/diff/1001/62#newcode14
Line 14: #locate file with float format test values
On 2009/04/09 10:52:04, Nick Coghlan wrote:
> The following if statement seems redundant.

Thanks!  Fixed in svn.  I'll upload changes to Rietveld later today.

http://codereview.appspot.com/33084/diff/1001/45
File Python/ast.c (right):

http://codereview.appspot.com/33084/diff/1001/45#newcode3165
Line 3165: PyFPE_START_PROTECT("strtod", return 0)
On 2009/04/09 10:52:04, Nick Coghlan wrote:
> Why change these from the atof form to the strtod-with-NULL form? Just a
> micro-optimisation to get rid of the redundant C function call?

More a rationalization than an optimization: PyOS_ascii_atof behaves in
some ways more like the standard strtod than like atof:  for example, it
sets errno.

I'll change it back, since this change is not necessary right now.

But I'd like to conisder possibly deprecating PyOS_ascii_atof.
One point to consider is that with David Gay's code, PyOS_ascii_atof *can* fail
due to lack of memory (though this seems very unlikely to happen in practice). 
There should probably be some checks for this here.

http://codereview.appspot.com/33084/diff/1001/47
File Python/marshal.c (right):

http://codereview.appspot.com/33084/diff/1001/47#newcode669
Line 669: PyFPE_START_PROTECT("strtod", break)
On 2009/04/09 10:52:04, Nick Coghlan wrote:
> Again with the change of an atof call to a direct strtod call. As with ast.c,
> not wrong but also not really relevant to the current task.

I'll change this back.

[Benjamin, 2009-04-14 00:42:20]

I didn't look in detail, but here are my nits.

http://codereview.appspot.com/33084/diff/3001/3028
File Makefile.pre.in (right):

http://codereview.appspot.com/33084/diff/3001/3028#newcode604
Line 604: PYTHON_HEADERS= \
The header file you added needs to go here.

http://codereview.appspot.com/33084/diff/3001/3005
File Python/marshal.c (right):

http://codereview.appspot.com/33084/diff/3001/3005#newcode710
Line 710: c.real = PyOS_ascii_atof(buf);
The indentation looks weird here.

[dickinsm, 2009-04-14 08:04:06]

http://codereview.appspot.com/33084/diff/3001/3028
File Makefile.pre.in (right):

http://codereview.appspot.com/33084/diff/3001/3028#newcode604
Line 604: PYTHON_HEADERS= \
On 2009/04/14 00:42:20, Benjamin wrote:
> The header file you added needs to go here.

Thanks, Benjamin!  Fixed in r71597.

http://codereview.appspot.com/33084/diff/3001/3005
File Python/marshal.c (right):

http://codereview.appspot.com/33084/diff/3001/3005#newcode710
Line 710: c.real = PyOS_ascii_atof(buf);
On 2009/04/14 00:42:20, Benjamin wrote:
> The indentation looks weird here.

So it does.  Fixed in r71597.

[ericvsmith, 2009-04-14 22:50:47]

http://codereview.appspot.com/33084/diff/1001/63
File Lib/test/test_types.py (right):

http://codereview.appspot.com/33084/diff/1001/63#newcode471
Line 471: #  after the py3k-short-float-repr merge
I've checked this in. It will be part of the merge to py3k. You can look at the
latest set of diffs to see it implemented.