From f2c2ef1f0e4b9fa260179fc53a5995ec93ee2ed6 Mon Sep 17 00:00:00 2001 From: neis Date: Wed, 20 Jul 2016 02:31:58 -0700 Subject: [PATCH] Make toLocaleString on arrays always call toLocaleString on its elements. As required by the spec. This is a variant of what I reverted in f47e7224034261bf62167a36959d5e287c4fc136. It will probably still cause a regression but now it's easier to migrate (parts of) the current implementation to C++, which is expected to make things faster again. BUG=chromium:627729,v8:5113 Review-Url: https://codereview.chromium.org/2164923002 Cr-Commit-Position: refs/heads/master@{#37897} --- src/js/array.js | 74 +++++++------------------ test/mjsunit/array-tostring.js | 24 +++++++- test/mjsunit/es6/typedarray-tostring.js | 13 +++++ test/test262/test262.status | 11 ---- 4 files changed, 57 insertions(+), 65 deletions(-) diff --git a/src/js/array.js b/src/js/array.js index c29b8f7256..c28f96e8d9 100644 --- a/src/js/array.js +++ b/src/js/array.js @@ -50,9 +50,9 @@ function KeySortCompare(a, b) { function GetSortedArrayKeys(array, indices) { if (IS_NUMBER(indices)) { - var keys = new InternalArray(); // It's an interval var limit = indices; + var keys = new InternalArray(); for (var i = 0; i < limit; ++i) { var e = array[i]; if (!IS_UNDEFINED(e) || i in array) { @@ -65,26 +65,24 @@ function GetSortedArrayKeys(array, indices) { } -function SparseJoinWithSeparatorJS(array, keys, length, convert, separator) { +function SparseJoinWithSeparatorJS(array, keys, length, use_locale, separator) { var keys_length = keys.length; var elements = new InternalArray(keys_length * 2); for (var i = 0; i < keys_length; i++) { var key = keys[i]; - var e = array[key]; elements[i * 2] = key; - elements[i * 2 + 1] = IS_STRING(e) ? e : convert(e); + elements[i * 2 + 1] = ConvertToString(use_locale, array[key]); } return %SparseJoinWithSeparator(elements, length, separator); } // Optimized for sparse arrays if separator is ''. -function SparseJoin(array, keys, convert) { +function SparseJoin(array, keys, use_locale) { var keys_length = keys.length; var elements = new InternalArray(keys_length); for (var i = 0; i < keys_length; i++) { - var e = array[keys[i]]; - elements[i] = IS_STRING(e) ? e : convert(e); + elements[i] = ConvertToString(use_locale, array[keys[i]]); } return %StringBuilderConcat(elements, keys_length, ''); } @@ -137,60 +135,38 @@ function StackHas(stack, v) { // join invocations. var visited_arrays = new Stack(); -function DoJoin(array, length, is_array, separator, convert) { +function DoJoin(array, length, is_array, separator, use_locale) { if (UseSparseVariant(array, length, is_array, length)) { %NormalizeElements(array); var keys = GetSortedArrayKeys(array, %GetArrayKeys(array, length)); if (separator === '') { if (keys.length === 0) return ''; - return SparseJoin(array, keys, convert); + return SparseJoin(array, keys, use_locale); } else { - return SparseJoinWithSeparatorJS(array, keys, length, convert, separator); + return SparseJoinWithSeparatorJS( + array, keys, length, use_locale, separator); } } // Fast case for one-element arrays. if (length === 1) { - var e = array[0]; - return IS_STRING(e) ? e : convert(e); + return ConvertToString(use_locale, array[0]); } // Construct an array for the elements. var elements = new InternalArray(length); + for (var i = 0; i < length; i++) { + elements[i] = ConvertToString(use_locale, array[i]); + } - // We pull the empty separator check outside the loop for speed! if (separator === '') { - for (var i = 0; i < length; i++) { - var e = array[i]; - elements[i] = IS_STRING(e) ? e : convert(e); - } return %StringBuilderConcat(elements, length, ''); - } - // Non-empty separator case. - // If the first element is a number then use the heuristic that the - // remaining elements are also likely to be numbers. - var e = array[0]; - if (IS_NUMBER(e)) { - elements[0] = %_NumberToString(e); - for (var i = 1; i < length; i++) { - e = array[i]; - if (IS_NUMBER(e)) { - elements[i] = %_NumberToString(e); - } else { - elements[i] = IS_STRING(e) ? e : convert(e); - } - } } else { - elements[0] = IS_STRING(e) ? e : convert(e); - for (var i = 1; i < length; i++) { - e = array[i]; - elements[i] = IS_STRING(e) ? e : convert(e); - } + return %StringBuilderJoin(elements, length, separator); } - return %StringBuilderJoin(elements, length, separator); } -function Join(array, length, separator, convert) { +function Join(array, length, separator, use_locale) { if (length === 0) return ''; var is_array = IS_ARRAY(array); @@ -204,7 +180,7 @@ function Join(array, length, separator, convert) { // Attempt to convert the elements. try { - return DoJoin(array, length, is_array, separator, convert); + return DoJoin(array, length, is_array, separator, use_locale); } finally { // Make sure to remove the last element of the visited array no // matter what happens. @@ -213,15 +189,9 @@ function Join(array, length, separator, convert) { } -function ConvertToString(x) { +function ConvertToString(use_locale, x) { if (IS_NULL_OR_UNDEFINED(x)) return ''; - return TO_STRING(x); -} - - -function ConvertToLocaleString(e) { - if (IS_NULL_OR_UNDEFINED(e)) return ''; - return TO_STRING(e.toLocaleString()); + return TO_STRING(use_locale ? x.toLocaleString() : x); } @@ -368,7 +338,7 @@ function ArrayToString() { if (IS_ARRAY(this)) { func = this.join; if (func === ArrayJoin) { - return Join(this, this.length, ',', ConvertToString); + return Join(this, this.length, ',', false); } array = this; } else { @@ -383,9 +353,7 @@ function ArrayToString() { function InnerArrayToLocaleString(array, length) { - var len = TO_LENGTH(length); - if (len === 0) return ""; - return Join(array, len, ',', ConvertToLocaleString); + return Join(array, TO_LENGTH(length), ',', true); } @@ -410,7 +378,7 @@ function InnerArrayJoin(separator, array, length) { return TO_STRING(e); } - return Join(array, length, separator, ConvertToString); + return Join(array, length, separator, false); } diff --git a/test/mjsunit/array-tostring.js b/test/mjsunit/array-tostring.js index 5be3d5045c..382bf8d7a0 100644 --- a/test/mjsunit/array-tostring.js +++ b/test/mjsunit/array-tostring.js @@ -125,7 +125,9 @@ var la1 = [1, [2, 3], 4]; assertEquals("1,2,3,4", la1.toLocaleString()); // Used on a string (which looks like an array of characters). -String.prototype.toLocaleString = Array.prototype.toLocaleString; +String.prototype.toLocaleString = function() { + return (this.length == 1) ? this : Array.prototype.toLocaleString.call(this); +} assertEquals("1,2,3,4", "1234".toLocaleString()); // If toLocaleString of element is not callable, throw a TypeError. @@ -157,3 +159,23 @@ for (var i = 0; i < 3; i++) { } Number.prototype.arrayToLocaleString = Array.prototype.toLocaleString; assertEquals("42,42,42", (42).arrayToLocaleString()); + + +(function TestToLocaleStringCalls() { + let log = []; + let pushArgs = (label) => (...args) => log.push(label, args); + + let NumberToLocaleString = Number.prototype.toLocaleString; + let StringToLocaleString = String.prototype.toLocaleString; + let ObjectToLocaleString = Object.prototype.toLocaleString; + Number.prototype.toLocaleString = pushArgs("Number"); + String.prototype.toLocaleString = pushArgs("String"); + Object.prototype.toLocaleString = pushArgs("Object"); + + [42, "foo", {}].toLocaleString(); + assertEquals(["Number", [], "String", [], "Object", []], log); + + Number.prototype.toLocaleString = NumberToLocaleString; + String.prototype.toLocaleString = StringToLocaleString; + Object.prototype.toLocaleString = ObjectToLocaleString; +})(); diff --git a/test/mjsunit/es6/typedarray-tostring.js b/test/mjsunit/es6/typedarray-tostring.js index e6adda0405..9d49cb1cc9 100644 --- a/test/mjsunit/es6/typedarray-tostring.js +++ b/test/mjsunit/es6/typedarray-tostring.js @@ -83,4 +83,17 @@ for (var constructor of typedArrayConstructors) { assertEquals("1,2", Array.prototype.join.call(a5)); assertEquals("1,2,3", Array.prototype.toString.call(a5)); assertEquals("1,2", Array.prototype.toLocaleString.call(a5)); + + (function TestToLocaleStringCalls() { + let log = []; + let pushArgs = (label) => (...args) => log.push(label, args); + + let NumberToLocaleString = Number.prototype.toLocaleString; + Number.prototype.toLocaleString = pushArgs("Number"); + + (new constructor([1, 2])).toLocaleString(); + assertEquals(["Number", [], "Number", []], log); + + Number.prototype.toLocaleString = NumberToLocaleString; + })(); } diff --git a/test/test262/test262.status b/test/test262/test262.status index a1737f5054..3e1eb4ea56 100644 --- a/test/test262/test262.status +++ b/test/test262/test262.status @@ -335,17 +335,6 @@ # https://bugs.chromium.org/p/v8/issues/detail?id=5012 'intl402/Intl/getCanonicalLocales/*': [FAIL], - # https://bugs.chromium.org/p/v8/issues/detail?id=5113 - 'built-ins/TypedArray/prototype/toLocaleString/calls-tolocalestring-from-each-value': [FAIL], - 'built-ins/TypedArray/prototype/toLocaleString/calls-tostring-from-each-value': [FAIL], - 'built-ins/TypedArray/prototype/toLocaleString/calls-valueof-from-each-value': [FAIL], - 'built-ins/TypedArray/prototype/toLocaleString/return-abrupt-from-firstelement-tolocalestring': [FAIL], - 'built-ins/TypedArray/prototype/toLocaleString/return-abrupt-from-firstelement-tostring': [FAIL], - 'built-ins/TypedArray/prototype/toLocaleString/return-abrupt-from-firstelement-valueof': [FAIL], - 'built-ins/TypedArray/prototype/toLocaleString/return-abrupt-from-nextelement-tolocalestring': [FAIL], - 'built-ins/TypedArray/prototype/toLocaleString/return-abrupt-from-nextelement-tostring': [FAIL], - 'built-ins/TypedArray/prototype/toLocaleString/return-abrupt-from-nextelement-valueof': [FAIL], - # https://bugs.chromium.org/p/v8/issues/detail?id=5115 'language/statements/class/subclass/class-definition-null-proto-missing-return-override': [FAIL], 'language/statements/class/subclass/class-definition-null-proto-this': [FAIL],