From b5fe75f9502a3fb582029ca6afdf38550531524f Mon Sep 17 00:00:00 2001 From: "kasperl@chromium.org" Date: Wed, 29 Oct 2008 10:02:09 +0000 Subject: [PATCH] Fix issue with Array.concat not preserving holes in the top-level arrays. Review URL: http://codereview.chromium.org/8694 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@635 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/factory.cc | 6 ++++++ src/factory.h | 6 +++++- src/objects.h | 3 --- src/runtime.cc | 26 +++++++++++++++----------- test/mjsunit/array-concat.js | 11 +++++++++++ 5 files changed, 37 insertions(+), 15 deletions(-) diff --git a/src/factory.cc b/src/factory.cc index 9db9fbbb2..0adf25899 100644 --- a/src/factory.cc +++ b/src/factory.cc @@ -42,6 +42,12 @@ Handle Factory::NewFixedArray(int size, PretenureFlag pretenure) { } +Handle Factory::NewFixedArrayWithHoles(int size) { + ASSERT(0 <= size); + CALL_HEAP_FUNCTION(Heap::AllocateFixedArrayWithHoles(size), FixedArray); +} + + Handle Factory::NewDictionary(int at_least_space_for) { ASSERT(0 <= at_least_space_for); CALL_HEAP_FUNCTION(Dictionary::Allocate(at_least_space_for), Dictionary); diff --git a/src/factory.h b/src/factory.h index abd84d30b..0cd80f069 100644 --- a/src/factory.h +++ b/src/factory.h @@ -37,10 +37,14 @@ namespace v8 { namespace internal { class Factory : public AllStatic { public: - // Allocate a new fixed array. + // Allocate a new fixed array with undefined entries. static Handle NewFixedArray( int size, PretenureFlag pretenure = NOT_TENURED); + + // Allocate a new fixed array with non-existing entries (the hole). + static Handle NewFixedArrayWithHoles(int size); + static Handle NewDictionary(int at_least_space_for); static Handle NewDescriptorArray(int number_of_descriptors); diff --git a/src/objects.h b/src/objects.h index d627935ed..61a7b92d2 100644 --- a/src/objects.h +++ b/src/objects.h @@ -1975,9 +1975,6 @@ class Dictionary: public DictionaryBase { // Fill in details for properties into storage. void CopyKeysTo(FixedArray* storage); - // Returns the value at entry. - static int ValueIndexFor(int entry) { return EntryToIndex(entry)+1; } - // For transforming properties of a JSObject. Object* TransformPropertiesToFastFor(JSObject* obj, int unused_property_fields); diff --git a/src/runtime.cc b/src/runtime.cc index 1a15ab0ad..07e8332b3 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -4128,16 +4128,17 @@ static uint32_t IterateArrayAndPrototypeElements(Handle array, /** * A helper function of Runtime_ArrayConcat. * - * The first argument is an Array of Arrays and objects. It is the same as - * the arguments array of Array::concat JS function. + * The first argument is an Array of arrays and objects. It is the + * same as the arguments array of Array::concat JS function. * - * If an argument is an Array object, the function visits array elements. - * If an argument is not an Array object, the function visits the object - * as if it is an one-element array. + * If an argument is an Array object, the function visits array + * elements. If an argument is not an Array object, the function + * visits the object as if it is an one-element array. * - * If the result array index overflows 32-bit integer, the rounded non-negative - * number is used as new length. For example, if one array length is 2^32 - 1, - * second array length is 1, the concatenated array length is 0. + * If the result array index overflows 32-bit integer, the rounded + * non-negative number is used as new length. For example, if one + * array length is 2^32 - 1, second array length is 1, the + * concatenated array length is 0. */ static uint32_t IterateArguments(Handle arguments, ArrayConcatVisitor* visitor) { @@ -4201,13 +4202,16 @@ static Object* Runtime_ArrayConcat(Arguments args) { Handle result = Factory::NewJSArray(0); uint32_t estimate_nof_elements = IterateArguments(arguments, NULL); - // If estimated number of elements is more than half of length, - // A fixed array (fast case) is more time & space-efficient than a dictionary. + // If estimated number of elements is more than half of length, a + // fixed array (fast case) is more time and space-efficient than a + // dictionary. bool fast_case = (estimate_nof_elements * 2) >= result_length; Handle storage; if (fast_case) { - storage = Factory::NewFixedArray(result_length); + // The backing storage array must have non-existing elements to + // preserve holes across concat operations. + storage = Factory::NewFixedArrayWithHoles(result_length); } else { // TODO(126): move 25% pre-allocation logic into Dictionary::Allocate diff --git a/test/mjsunit/array-concat.js b/test/mjsunit/array-concat.js index 22e02a118..2346c8de6 100644 --- a/test/mjsunit/array-concat.js +++ b/test/mjsunit/array-concat.js @@ -107,3 +107,14 @@ c = a.concat('Hello'); assertEquals(1, c.length); assertEquals("Hello", c[0]); assertEquals("Hello", c.toString()); + +// Check that concat preserves holes. +var holey = [void 0,'a',,'c'].concat(['d',,'f',[0,,2],void 0]) +assertEquals(9, holey.length); // hole in embedded array is ignored +for (var i = 0; i < holey.length; i++) { + if (i == 2 || i == 5) { + assertFalse(i in holey); + } else { + assertTrue(i in holey); + } +} -- 2.34.1