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

diff --git a/src/factory.cc b/src/factory.cc
index 9db9fbb..0adf258 100644 (file)
--- a/src/factory.cc
+++ b/src/factory.cc
@@ -42,6 +42,12 @@ Handle<FixedArray> Factory::NewFixedArray(int size, PretenureFlag pretenure) {
 }
 
 
+Handle<FixedArray> Factory::NewFixedArrayWithHoles(int size) {
+  ASSERT(0 <= size);
+  CALL_HEAP_FUNCTION(Heap::AllocateFixedArrayWithHoles(size), FixedArray);
+}
+
+
 Handle<Dictionary> 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 abd84d3..0cd80f0 100644 (file)
--- 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<FixedArray> NewFixedArray(
       int size,
       PretenureFlag pretenure = NOT_TENURED);
+
+  // Allocate a new fixed array with non-existing entries (the hole).
+  static Handle<FixedArray> NewFixedArrayWithHoles(int size);
+
   static Handle<Dictionary> NewDictionary(int at_least_space_for);
 
   static Handle<DescriptorArray> NewDescriptorArray(int number_of_descriptors);

diff --git a/src/objects.h b/src/objects.h
index d627935..61a7b92 100644 (file)
--- 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 1a15ab0..07e8332 100644 (file)
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -4128,16 +4128,17 @@ static uint32_t IterateArrayAndPrototypeElements(Handle<JSArray> 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<JSArray> arguments,
                                  ArrayConcatVisitor* visitor) {
@@ -4201,13 +4202,16 @@ static Object* Runtime_ArrayConcat(Arguments args) {
   Handle<JSArray> 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<FixedArray> 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 22e02a1..2346c8d 100644 (file)
--- 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);
+  }
+}