Closed Bug 442974 Opened 16 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

(Not tracked)

RESOLVED WONTFIX
Q1 12 - Brannan

People

(Reporter: dlollar, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: AS3_Builtins
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → Future
Priority: P3 → --
Target Milestone: Future → ---
Still alive and well.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → flash10.1
Priority: P3 → P4
Blocks: a2d-d2a
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
TC pushed: redux changeset: 2760:2e75d5d3010b
Attached patch Patch (obsolete) (deleted) — Splinter Review
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.
Attachment #405835 - Flags: review?(stejohns)
Comment on attachment 405835 [details] [diff] [review] Patch I like the fix, but it might need version-checking for Flash...
Attachment #405835 - Flags: review?(stejohns) → review+
Whiteboard: Has patch
Whiteboard: Has patch → Has patch; versioning
Flags: flashplayer-needsversioning+
Whiteboard: Has patch; versioning → Has patch
Assignee: lhansen → nobody
Priority: P4 → --
Target Milestone: flash10.1 → Future
Depends on: 535770
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.
Status: ASSIGNED → NEW
Priority: -- → P2
Whiteboard: Has patch → has-patch
Target Milestone: Future → flash10.2
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
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.
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.)
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.
Attachment #465203 - Flags: feedback?(edwsmith)
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)
Attachment #465203 - Flags: feedback?(edwsmith) → feedback+
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 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.
Attachment #405835 - Attachment is obsolete: true
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)
Whiteboard: has-patch → has-patch must–fix-candidate
Whiteboard: has-patch must–fix-candidate → has-patch must-fix-candidate
Flags: flashplayer-injection-
Flags: flashplayer-bug+
Whiteboard: has-patch must-fix-candidate
Target Milestone: Q3 11 - Serrano → Q1 12 - Brannan
Assignee: lhansen → nobody
Status: ASSIGNED → NEW
Tamarin is a dead project now. Mass WONTFIX.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Tamarin isn't maintained anymore. WONTFIX remaining bugs.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: