Refactor IterationHasObservableEffects to be more readable.

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index 9f824fbf482bd73fb0769625fec091a2d78b6019..e9cec3a210cd29cf0866c8f6c01756f8a7e29a79 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -2177,33 +2177,43 @@ MaybeHandle<Object> Object::ArraySpeciesConstructor(
 }
 
 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();
-
-    // 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 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 this object is an array.
+  if (!IsJSArray()) return true;
+  JSArray* spread_array = JSArray::cast(this);
+  Isolate* isolate = spread_array->GetIsolate();
+
+  // 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 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.
+
+  // 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;
+  } else {

[Franzi, 2016/12/15 14:39:26]

If you return inside an if, you don't need an else. 

if () {
  ... 
  return false;
}
return true;

+    return true;
   }
-  return true;
 }
 
 void Object::ShortPrint(FILE* out) {