108305 - Upgrade to the current version of dtoa.c

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement, P2)

Description

[Wan-Teh Chang]

NSPR's prdtoa.c is based on David M. Gay's dtoa.c.
The author of dtoa.c informed us that there have
been various bug fixes since 1991, so it might be
wise to update to the current version (which has
the same license, with "AT&T" changed to "Lucent"
to reflect the AT&T breakup of the mid 1990's).
It's

    http://www.netlib.org/fp/dtoa.c
or
    http://netlib.bell-labs.com/netlib/fp/dtoa.c.gz
or
    ftp://netlib.bell-labs.com/netlib/fp/dtoa.c.gz

with bug fixes summarized in fp/changes, i.e.,

    http://www.netlib.org/fp/changes
or
    http://netlib.bell-labs.com/netlib/fp/changes.gz
or
    ftp://netlib.bell-labs.com/netlib/fp/changes.gz

There's also a generalization to other IEEE-like arithmetics,

    http://www.netlib.org/fp/gdtoa.tgz
or
    http://netlib.bell-labs.com/netlib/fp/gdtoa.tgz
or
    ftp://netlib.bell-labs.com/netlib/fp/gdtoa.tgz

Comment 1

[Brendan Eich [:brendan]]

Cc'ing khanson, in case the js/src/jsdtoa.c fork needs updating too.

/be

Comment 2

[Kenton Hanson (gone)]

I am working on bug #85267 (memory leak in jsdtoa.c).  I am very familar with
this code.  During my tenure at Apple I replaced all of dtoa.c in the Apple
operating system with code I wrote.

=Kenton=

Comment 3

[Wan-Teh Chang]

Attachment: prdtoa.c, based on dtoa.c as of Thu Mar 25 22:34:56 MST 2004

Incomplete.  Attached for reference.

Comment 4

[Wan-Teh Chang]

Attachment: prdtoa.c, based on dtoa.c as of Thu Mar 25 22:34:56 MST 2004

Added NSPR implicit initialization to PR_strtod and PR_dtoa.

Comment 5

[Wan-Teh Chang]

Attachment: Diffs between dtoa.c and prdtoa.c

Comment 6

[Wan-Teh Chang]

Attachment: prdtoa.c, based on dtoa.c as of Thu Mar 25 22:34:56 MST 2004

I reviewed all the changes we made to the old dtoa.c (the
original 1991 version) and applied the most of them to
the current dtoa.c.  The result is this attachment.

The changes I did not apply are:

1. Changing DEBUG to DEBUG_DTOA.

2. #define MALLOC PR_MALLOC
So MALLOC defaults to Standard C Library's malloc.

3. A lot of changes from int to PRInt32, which were
most likely for the WIN16 port.

4. Compiler warning fixes.  I'll compile dtoa.c under
gcc and MSVC and submit the warnings (and suggested
fixes) to the author of dtoa.c.

5. WIN16 Watcom compiler support.

6. Changing double to PRFloat64.

7. Changing the page numbers of the paper that inspired
dtoa from pp. 92-101 to pp.112-126.  I will submit this
change to the author of dtoa.c.

I also found a problem with dtoa.c, which I will
report to the author of dtoa.c:

- Balloc doesn't check for the failure of MALLOC.
  rv_alloc doesn't check for the failure of Balloc.
  nrv_alloc doesn't check for the failure of rv_alloc.

To do:
- Submit the gcc and MSVC compiler warnings, correct
  page numbers for the dtoa reference, and the above
  Balloc/rv_alloc/nrv_alloc problem to the author of
  dtoa.c.
- Carefully review dtoa.c's changes file to see if
  there are any incompatible changes.  For example,
  the meanings of some dtoa modes changed.
- Search in mozilla cvs repository to determine which
  dtoa modes we are using.
- Review the comments at the top of dtoa.c to see
  which macros we need to define.

Comment 7

[Wan-Teh Chang]

Attachment: Diffs between dtoa.c and prdtoa.c

Comment 8

[Wan-Teh Chang]

Other to do items:
- Submit our IEEE_ARM addition to the author of
  dtoa.c.
- Find out if JS still passes the ECMAScript tests
  when using the new prdtoa.c.  I need to work with
  a JS developer on this.
- Find out why NSPR's dtoa.c test program needs
  the following change in order to pass with the
  new prdtoa.c.

Index: dtoa.c
===================================================================
RCS file: /cvsroot/mozilla/nsprpub/pr/tests/dtoa.c,v
retrieving revision 1.4
diff -u -r1.4 dtoa.c
--- dtoa.c      20 Jun 2000 21:39:34 -0000      1.4
+++ dtoa.c      27 Mar 2004 16:20:00 -0000
@@ -67,7 +67,7 @@
     }

     PR_cnvtf(cnvt,sizeof(cnvt),20,num);
-    if(strcmp("1e+24",cnvt) != 0){
+    if(strcmp("1.e+24",cnvt) != 0){
        fprintf(stderr,"Failed to convert numeric value %lf %s\n",num,cnvt);
         failed_already = 1;
     }

Comment 9

[Wan-Teh Chang]

By browsing the mozilla cvs repository, I found
that:
1. Mozilla uses PR_dtoa in modes 0, 2, 3.
2. NSPR (PR_cnvtf) uses PR_dtoa in mode 1.
3. JS (JS_dtostr) uses js_dtoa in modes 0, 2, 3.

I will also suggest the author of dtoa.c to
rename FREE_DTOA_LOCK to RELEASE_DTOA_LOCK.

Comment 10

[Wan-Teh Chang]

Attachment: prdtoa.c, based on dtoa.c as of Thu Mar 25 22:34:56 MST 2004

Made PR_dtoa return the right value in *rve.  This bug
caused the test failure I described in comment 8.

Comment 11

[Wan-Teh Chang]

Attachment: Diffs between dtoa.c and prdtoa.c

Comment 12

[Wan-Teh Chang]

Attachment: prdtoa.c, based on dtoa.c as of Mon Apr 12 00:44:22 MDT 2004

Comment 13

[Wan-Teh Chang]

Attachment: Diffs between dtoa.c and prdtoa.c

Comment 14

[Wan-Teh Chang]

Comment on attachment 145977 [details] [diff] [review]
Diffs between dtoa.c and prdtoa.c

Darin, this patch represents all the changes we
make to the latest version of dtoa.c.

Comment 15

[Darin Fisher]

Comment on attachment 145977 [details] [diff] [review]
Diffs between dtoa.c and prdtoa.c

>+PR_IMPLEMENT(PRStatus)
>+PR_dtoa(PRFloat64 d, int mode, int ndigits,
>+	int *decpt, int *sign, char **rve, char *buf, PRSize bufsize)
>+{

should the parameters use PRIntn instead of int?  i know that PRIntn
is just a typedef for int, but perhaps it is good to keep this code
in sync with what appears in the header file?


>+    if (bufsize < strlen(result)+1) {
>+        PR_SetError(PR_BUFFER_OVERFLOW_ERROR, 0);
>+    } else {
>+        strcpy(buf, result);

perhaps it would be better to store the result of strlen and use
memcpy instead of strcpy.  i'd imagine that memcpy is faster than
strcpy since strcpy has to compare each byte before copying it.

    PRIntn resultLen = strlen(result)+1;
    if (bufsize < resultLen) {
	...
    } else {
	memcpy(buf, result, resultLen);


r=darin

Comment 16

[Wan-Teh Chang]

Patch checked in on the NSPR trunk (NSPR 4.6) and
NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 alpha).

Comment 17

[Wan-Teh Chang]

I forgot to mention that I made the two changes Darin
suggested in comment 15.