442974 - Number.toPrecision() sometimes outputs fixed-notation number when it should output exponential-notation number

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[David Lollar]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; .NET CLR 2.0.50727; InfoPath.1)
Build Identifier: 10_d517

Tracing out a number writes out many zeroes instead of using exponential notation.

Reproducible: Always

Steps to Reproduce:
1. Put this code in a program, and run it:
trace(Number.MIN_VALUE.toPrecision(21))
Actual Results:  
0.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
0000000000000000000000000494065645841246544177

Expected Results:  
The precise rules are, as I said, pretty obtuse; but on a gut level, toPrecision() is supposed to pick either fixed-notation or exponential-
notation, depending on which one is "better." In this case, other browsers output the exponential notation, which makes sense 
because the fixed notation is so long and is mostly zeroes.

Here is what Mozilla Firefox 3 outputs: 4.94065645841246544177e-324

(Safari gets its wrong, in an amusing way: I.nfinitye-322)

originally from 232400.

Comment 1

[Lars T Hansen]

Still alive and well.

Comment 2

[Lars T Hansen]

TC pushed: redux changeset:   2760:2e75d5d3010b

Comment 3

[Lars T Hansen]

Attachment: Patch

A local fix: as D2A::expBase10 returns -16 as the exponent for a value of '0', but we don't really want to change that for all functions, insert a simple check in the case for toPrecision: if the value is zero, set the exponent to zero.

After that, select exponential or normal formatting as per ECMA-262.

Comment 4

[Steven Johnson]

Comment on attachment 405835 [details] [diff] [review]
Patch

I like the fix, but it might need version-checking for Flash...

Comment 5

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 6

[Lars T Hansen]

Whack-a-mole: (1e-7).toPrecision(10) now formats as 0.e+1 (!) which is wrong in too many ways to count.  (Firefox says 1.000000000e-7)  The loop in the code that scans for firstNonZero / lastNonZero finds the decimal point from both ends, which may not have been the intent.

Comment 7

[Lars T Hansen]

Another bug:

ES5 says that if the value is NaN or Infinity then "NaN" or "Infinity" or "-Infinity" are returned even if the precision specifier is outside the allowed range, but in Tamarin the range check happens in AS3 code, which then calls _convert in C++ code to deal with the rest.

(Also the case for toExponential.)

Comment 8

[Lars T Hansen]

Attachment: Clean up MathUtils::convertDoubleToString (no bug fixes)

Claim: this version behaves the same as the old version, but it reduces the number of variables global in the function, makes more of them constant, does away with some confusion, and adds a lot of comments.

Comment 9

[Edwin Smith]

Comment on attachment 465203 [details] [diff] [review]
Clean up MathUtils::convertDoubleToString (no bug fixes)

I believe the claim.  notes from code review follow, but nothing important.

on line 1069, The test

  if (value)

was changed to:

  const bool zero = (value == 0);
  if (!zero)

if value is NaN, is this valid? (if(NaN) is not taken, IIRC). I see we handle NaN further up, so possibly moot.  are we safe for the other oddities (+/- Infinity, -0) ? I think so.  ((value < 0) is false for value == -0, so we can still have the sign bit set at this point, i think).

on line 1128 in the old code, the test (*ptr != 0x3A) became ('9'+1)... okay.  i'm like "why do we care about ';'?"  (answer:  man ascii)

the old code checked (*ptr < '0') to skip the decimal, new code uses (*ptr == '.').  did anything further up ensure the only character < '0' would be '.'?

in new code, line 1177, we have

  while (...)
      ;

using {} instead of ; for an empty block is more obvious, we dont have a rigid style gude but the ones i've read all suggest it.  (webkit, mozilla, google, llvm)

Comment 10

[Steven Johnson]

Comment on attachment 465203 [details] [diff] [review]
Clean up MathUtils::convertDoubleToString (no bug fixes)

nit: we should cache "NaN", "Infinity", "-Infinity" in AvmCore instead of calling newConstantStringLatin1() each time.

nit: code of the form:

	while (whatever)
		;

can trigger warnings ("possibly accidental semicolon") on anal-retentive compilers; something like

	while (whatever) { /* nothing */ }

is safer, alas...

Comment 11

[Lars T Hansen]

Comment on attachment 405835 [details] [diff] [review]
Patch

The real problem is that our D2A code does not implement the correct algorithms for fixed-precision formatting from Burger & Dybvig.  I have a modification which corrects for that in some cases by post-facto rounding of the output, but that's just not in the spirit of the original algorithm - it's supposed to generate correctly-rounded output.  There are old comments in the code to the effect that fixed-precision formatting is sometimes not right, so it appears to be a conscious oversight, not a bug.

Anyway the correct fix to this bug will be:

- a new implementation of toPrecision, under versioning control
- point-fixes to D2A to implement correct fixed-precision rounding, also under
  versioning control

The first part is easy, the second requires a little more work.

Similar fixes are likely for toFixed and toExponential and will certainly build on the same work.

Comment 12

[Chris Peyer]

Another case where toPrecision is wrong- using any value from 999995-1000000:

trace(Number (999994).toPrecision (5)); // 9.9999e+5
trace(Number (999995).toPrecision (5)); // 0.e+9
trace(Number (999998.4999).toPrecision (5)); // 0.e+9

(Examples come from https://bugs.adobe.com/jira/browse/FP-3481)

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.