Refactor IterationHasObservableEffects to be more readable.

src/objects.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/objects.h"
 
 #include <cmath>
 #include <iomanip>
 #include <memory>
 #include <sstream>
@@ skipping 2159 matching lines @@
     if (!constructor->IsConstructor()) {
       THROW_NEW_ERROR(isolate,
           NewTypeError(MessageTemplate::kSpeciesNotConstructor),
           Object);
     }
     return constructor;
   }
 }
 
 bool Object::IterationHasObservableEffects() {
-  if (IsJSArray()) {
-    // Check that the spread arg has fast elements
-    JSArray* spread_array = JSArray::cast(this);
-    ElementsKind array_kind = spread_array->GetElementsKind();
-    Isolate* isolate = spread_array->GetIsolate();
+  // Check that this object is an array.
+  if (!IsJSArray()) return true;
+  JSArray* spread_array = JSArray::cast(this);
+  Isolate* isolate = spread_array->GetIsolate();
 
-    // And that it has the orignal ArrayPrototype
-    JSObject* array_proto = JSObject::cast(spread_array->map()->prototype());
-    Map* iterator_map = isolate->initial_array_iterator_prototype()->map();
+  // Check that we have the original ArrayPrototype.
+  JSObject* array_proto = JSObject::cast(spread_array->map()->prototype());
+  if (!isolate->is_initial_array_prototype(array_proto)) return true;
 
-    // Check that the iterator acts as expected.
-    // If IsArrayIteratorLookupChainIntact(), then we know that the initial
-    // ArrayIterator is being used. If the map of the prototype has changed,
-    // then take the slow path.
-    if (isolate->is_initial_array_prototype(array_proto) &&
-        isolate->IsArrayIteratorLookupChainIntact() &&
-        isolate->is_initial_array_iterator_prototype_map(iterator_map)) {
-      if (IsFastPackedElementsKind(array_kind)) {
-        return false;
-      }
-      if (IsFastHoleyElementsKind(array_kind) &&
-          isolate->IsFastArrayConstructorPrototypeChainIntact()) {
-        return false;
-      }
-    }
+  // Check that the ArrayPrototype hasn't been modified in a way that would
+  // affect iteration.
+  if (!isolate->IsArrayIteratorLookupChainIntact()) return true;
+
+  // TODO(petermarshall): We probably need to add a protector to the initial
+  // array iterator just like the array prototype, because there are some
+  // changes that a map check won't recognize.

[Michael Starzinger, 2016/12/15 15:05:39]

question: Not sure about this TODO, which changes to the iterator do you have in
mind? If we are not sure what the concrete issue here is we could file an issue
on the issue tracker instead.

+
+  // Check that the map of the initial array iterator hasn't changed.
+  Map* iterator_map = isolate->initial_array_iterator_prototype()->map();
+  if (!isolate->is_initial_array_iterator_prototype_map(iterator_map)) {
+    return true;
+  }
+
+  // For FastPacked kinds, iteration will have the same effect as simply
+  // accessing each property in order.
+  ElementsKind array_kind = spread_array->GetElementsKind();
+  if (IsFastPackedElementsKind(array_kind)) return false;
+
+  // For FastHoley kinds, an element access on a hole would cause a lookup on
+  // the prototype. This could have different results if the prototype has been
+  // changed.
+  if (IsFastHoleyElementsKind(array_kind) &&
+      isolate->IsFastArrayConstructorPrototypeChainIntact()) {
+    return false;
   }
   return true;
 }
 
 void Object::ShortPrint(FILE* out) {
   OFStream os(out);
   os << Brief(this);
 }
 
 
@@ skipping 18196 matching lines @@
       // depend on this.
       return DICTIONARY_ELEMENTS;
     }
     DCHECK_LE(kind, LAST_ELEMENTS_KIND);
     return kind;
   }
 }
 
 }  // namespace internal
 }  // namespace v8