[webkit-reviews] review denied: [Bug 77018] implement Number.toLocaleString() using ICU : [Attachment 203420] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri May 2 15:17:48 PDT 2014


Liam Quinn <lquinn at blackberry.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 77018: implement Number.toLocaleString() using ICU
https://bugs.webkit.org/show_bug.cgi?id=77018

Attachment 203420: Patch
https://bugs.webkit.org/attachment.cgi?id=203420&action=review

------- Additional Comments from Liam Quinn <lquinn at blackberry.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=203420&action=review


> Source/JavaScriptCore/runtime/NumberPrototype.cpp:103
> +	   bufferLength = decimalNumber.toStringDecimal(numberString,
WTF::NumberToStringBufferLength);

DecimalNumber doesn't actually check the buffer length in release builds, and
WTF::NumberToStringBufferLength assumes exponential notation that
toStringDecimal() doesn't do. This would crash for large numbers and NaN.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:104
> +	   bufferLength = unum_formatDecimal(formatter, numberString,
bufferLength, buffer, WTF::NumberToStringBufferLength, 0, &status);

Is there a reason not to use unum_formatDouble instead of unum_formatDecimal?
Then you wouldn't need the DecimalNumber.


More information about the webkit-reviews mailing list