7625 - Number.toString(base != 10) not working for negative numbers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[martin.honnen]

Calling
  toString(base)
with a base != 10 on a negative number returns utter nonsense


js>  for (var n = -1; n > -11; n--) print (n.toString(16))
/
.
-
,
+
*
)
(
'
&


I guess that is not what ECMA calls implementation dependant. Happens
consistently with communicator on win95 and linux and with js1.4-2 (built on
linux).

Comment 1

[Norris Boyd]

Looks like a bug. Rhino appears to have the correct behavior.

Comment 2

[cbegle]

the behavior in this case is not actally specified in ecma though i agree it's
pretty crappy default behav ior.  here's what it says:

15.7.4.2 Number.prototype.toString(radix)
If the radix is the number 10 or not supplied, then this number value is given
as an argument to the ToString operator; the resulting string value is
returned.

If the radix is supplied and is an integer from 2 to 36, but not 10, the result
is a string, the choice of which is implementation-dependent.

The toString function is not generic; it generates a runtime error if its this
value is not a Number object. Therefore, it cannot be transferred to other
kinds of objects for use as a method.

Comment 3

[martin.honnen]

Are you using another rhino than the one available as rhinotip on mozilla.org as
that one doesn't seem to have
  Number.toString(base)
but simply ignores any argument.
So what is correct behaviour for negative numbers, what
  java.lang.Integer.toString(n, base)
does (taking the abs value and prefixing with - if needed) or what
  java.lang.Integer.toHex/Binary/OctalString(n)
does (regarding the number as an unsigned int and converting to base 16/8/2)?
JScript seems to use the abs value and prefix with - if needed. Probably easier
to implement/add as a fix.

Comment 4

[martin.honnen]

I tried to fix Number.toString to treat the Number as unsigned (as
java.lang.Integer.toHexString() does) to get meaningful results for negative
numbers and it seems to work:

static JSBool
num_toString(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
{
    jsval v;
    jsdouble d;
    jsint base, dval;
    unsigned int ival;  // changed that from jsint to unsigned int
    char *bp, buf[32];
    JSString *str;



    if (!JS_InstanceOf(cx, obj, &number_class, argv))
	return JS_FALSE;
    v = OBJ_GET_SLOT(cx, obj, JSSLOT_PRIVATE);
    if (!JSVAL_IS_NUMBER(v))
	return js_obj_toString(cx, obj, argc, argv, rval);
    d = JSVAL_IS_INT(v) ? (jsdouble)JSVAL_TO_INT(v) : *JSVAL_TO_DOUBLE(v);
    if (argc != 0) {
	if (!js_ValueToECMAInt32(cx, argv[0], &base))
	    return JS_FALSE;
	if (base < 2 || base > 36) {
	    char numBuf[12];
	    JS_snprintf(numBuf, sizeof numBuf, "%ld", (long) base);
	    JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_RADIX,
	    			 numBuf);
	    return JS_FALSE;
	}
	if (base != 10 && JSDOUBLE_IS_FINITE(d)) {
	    ival = (unsigned int) js_DoubleToInteger(d); // changed the cast here

	    bp = buf + sizeof buf;
	    for (*--bp = '\0'; ival != 0 && --bp >= buf; ival /= base) {
		dval = ival % base;
		*bp = (char)((dval >= 10) ? 'a' - 10 + dval : '0' + dval);
	    }
	    if (*bp == '\0')
		*--bp = '0';


	    str = JS_NewStringCopyZ(cx, bp);
	} else {
	    str = js_NumberToString(cx, d);
	}
    } else {
	str = js_NumberToString(cx, d);
    }
    if (!str)
	return JS_FALSE;
    *rval = STRING_TO_JSVAL(str);
    return JS_TRUE;
}

That seems to improve things for negative numbers without changing anything for
positive
js> for (var n = -1; n > -20; n--) print(n + ': ' + n.toString(8))
-1: 37777777777
-2: 37777777776
-3: 37777777775
-4: 37777777774
-5: 37777777773
-6: 37777777772
-7: 37777777771
-8: 37777777770
-9: 37777777767
-10: 37777777766
-11: 37777777765
-12: 37777777764
-13: 37777777763
-14: 37777777762
-15: 37777777761
-16: 37777777760
-17: 37777777757
-18: 37777777756
-19: 37777777755


js> for (var n = -1; n > -20; n--) print(n + ': ' + n.toString(16))
-1: ffffffff
-2: fffffffe
-3: fffffffd
-4: fffffffc
-5: fffffffb
-6: fffffffa
-7: fffffff9
-8: fffffff8
-9: fffffff7
-10: fffffff6
-11: fffffff5
-12: fffffff4
-13: fffffff3
-14: fffffff2
-15: fffffff1
-16: fffffff0
-17: ffffffef
-18: ffffffee
-19: ffffffed

Comment 5

[Norris Boyd]

You're right that Rhino ignores the argument to Number.prototype.toString
entirely. That's a bug that should be fixed.

Comment 6

[cbegle]

moving to the "JavaScript Engine" component.

Comment 7

[rogerl (gone)]

Fixed in SpiderMonkey & Rhino by returning '-' + toString(abs(n), base) for
negative numbers.

Comment 8

[cbegle]

js> (-10).toString(16)
-a

this still seems kind of wrong -- seems like it should return "-0xa".  if we
don't, we get this:

js> (-10).toString(16)
-a
js> Number( (-10).toString(16) )
NaN
js> Number( "-0xa" )
-10

Comment 9

[rogerl (gone)]

Gets uglier by the minute. According to my reading, Number("-0xa") should have
returned NaN since a "-" is only allowed to precede a decimal literal. In which
case, maybe I should back out the change in favor of treating bases other than
10 as unsigned.

Comment 10

[cbegle]

wait a sec -- who're you saying is getting uglier by the minute?!

Comment 11

[cbegle]

okay, i take it back.  making it hex is not right.
js> (10).toString(16)

its been a while since i've been in the spec.

Comment 12

[leger]

Clearing Fixed resolution due to ReOpen of this bug.

Comment 13

[leger]

Moving from M7 to M9.  M7 is long gone :-)

Comment 14

[rogerl (gone)]

O.K. since you changed your mind, I'm going to stick by my original decision to
treat the value as signed and just leave it this way.