521772 - Clean up the API for toFixed, toPrecision, and toExponential

Status: RESOLVED WONTFIX

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

Description

[Lars T Hansen]

The current implementation is not according to ECMA-262: the default argument values are 0, but in some cases need to be undefined because those two values are distinguished (undefined means "as many digits as necessary" and 0 means "zero").  The distinction is lost and must be recovered, and then passed on to the underlying machinery.

Also, at the highest level these methods are incorrectly implemented.  Both toExponential and toPrecision will return NaN/Infinity values regardless of the parameter value, ie, the error check currently happens too soon.

Comment 1

[Lars T Hansen]

Attachment: Patch

Patch is big because it contains a change to builtin.cpp.

Several levels of fix here:
 - Number.as is changed to distinguish between 'undefined'/'not supplied' and
   '0' arguments, as required by E262-3
 - NumberClass.cpp is changed to make use of that distinction, and to signal
   errors for toPrecision/toExponential later, in accordance with E262-3
 - MathUtils.cpp is changed to make use of the undefined/0 distinction; as a
   side effect we fix bug #478804.

One reason for disliking this patch might be that it fails to correctly report
the value of an illegal precision/fractionDigits parameter when it throws an
error, because the precision argument to Number::convert is changed by the AS3
code.  If this is seen as an important problem then it can be fixed, either by
passing that value as an atom or by adding a parameter.

Comment 2

[Steven Johnson]

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

I actually like the patch, but I am rejecting only because I think we have to figure out whether or not this needs to be version-checked for Flash compatibility (and if so the patch will need some reworking).

Comment 3

[Steven Johnson]

Any fix for this bug will likely need version-checking for backwards compatibility.

Comment 4

[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 5

[Steven Johnson]

I grabbed this bug and looked at applying BugCompatibility to it, but unfortunately it's tricky: there's no way to use BugCompatibility to change the value of default arguments, and we can't distinguish a filled-in vs explicitly-specified value of void(0) as the argument... it would be painful to add customization of default args to the BugCompatibility framework, so, looking for another approach, two things come to mind, neither of which are particularly appealing...


	AS3 function toPrecision(...):String
	{
		if (arguments.length > 1) throw(kWrongArgCountError);

		var p;
		if (arguments.length == 0)
			p = bugzilla(521772) ? void(0) : 0;
		else
			p = arguments[0];
		// rest of logic
	}

	AS3 function toPrecision(p="constantStringWeAssumeWillNeverBePassed"):String
	{
		if (p === "constantStringWeAssumeWillNeverBePassed")
			p = bugzilla(521772) ? void(0) : 0;
		// rest of logic
	}


thoughts?

Comment 6

[Lars T Hansen]

Actually the first of your solutions is fine, if it's cleaned up a little (you can't use both ... and 'arguments', and you don't want to use 'arguments' in any case), because the rest arguments optimization will prevent any storage allocation from occurring:

  AS3 function toPrecision(...rest): String {
      if (rest.length > 1) throw(...);
      var p;
      if (rest.length == 0)
          p = ...;
      else
          p = rest[0];
      ...;
  }

Comment 7

[Trevor Baker]

updating milestone to Serrano as the two dependent issues were targeted to Serrano.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.