Split ArrayData::append(ArrayOp) into plus() and merge()

We had two distinct functions dispatched by an enum that was
always known somewhere higher up on the callstack.

hphp/runtime/base/array/array_data.h

@@ -36,11 +36,6 @@ class ArrayData : public Countable {
  public:
   enum class AllocationMode : bool { smart, nonSmart };
 
-  enum ArrayOp {
-    Plus,
-    Merge,
-  };
-
   // enum of possible array types, so we can guard nonvirtual
   // fast paths in runtime code.
   enum class ArrayKind : uint8_t {
@@ -403,7 +398,8 @@ public:
    * first then append/merge arrays. Return NULL if escalation is not needed,
    * or an escalated array data.
    */
-  virtual ArrayData *append(const ArrayData *elems, ArrayOp op, bool copy) = 0;
+  virtual ArrayData *plus(const ArrayData *elems, bool copy) = 0;
+  virtual ArrayData *merge(const ArrayData *elems, bool copy) = 0;
 
   /**
    * Stack function: pop the last item and return it.

hphp/runtime/base/array/hphp_array.cpp

@@ -1415,31 +1415,32 @@ ArrayData *HphpArray::appendWithRef(CVarRef v, bool copy) {
   return a->nextInsertWithRef(v);
 }
 
-ArrayData* HphpArray::append(const ArrayData* elems, ArrayOp op, bool copy) {
+ArrayData* HphpArray::plus(const ArrayData* elems, bool copy) {
   HphpArray* a = !copy ? this : copyImpl();
-  if (op == Plus) {
-    for (ArrayIter it(elems); !it.end(); it.next()) {
-      Variant key = it.first();
-      CVarRef value = it.secondRef();
-      if (key.isNumeric()) {
-        a->addValWithRef(key.toInt64(), value);
-      } else {
-        a->addValWithRef(key.getStringData(), value);
-      }
+  for (ArrayIter it(elems); !it.end(); it.next()) {
+    Variant key = it.first();
+    CVarRef value = it.secondRef();
+    if (key.isNumeric()) {
+      a->addValWithRef(key.toInt64(), value);
+    } else {
+      a->addValWithRef(key.getStringData(), value);
     }
-  } else {
-    assert(op == Merge);
-    for (ArrayIter it(elems); !it.end(); it.next()) {
-      Variant key = it.first();
-      CVarRef value = it.secondRef();
-      if (key.isNumeric()) {
-        a->nextInsertWithRef(value);
-      } else {
-        Variant *p;
-        StringData *sd = key.getStringData();
-        a->addLvalImpl(sd, sd->hash(), &p);
-        p->setWithRef(value);
-      }
+  }
+  return a;
+}
+
+ArrayData* HphpArray::merge(const ArrayData* elems, bool copy) {
+  HphpArray* a = !copy ? this : copyImpl();
+  for (ArrayIter it(elems); !it.end(); it.next()) {
+    Variant key = it.first();
+    CVarRef value = it.secondRef();
+    if (key.isNumeric()) {
+      a->nextInsertWithRef(value);
+    } else {
+      Variant *p;
+      StringData *sd = key.getStringData();
+      a->addLvalImpl(sd, sd->hash(), &p);
+      p->setWithRef(value);
     }
   }
   return a;

hphp/runtime/base/array/hphp_array.h

@@ -159,7 +159,8 @@ public:
   ArrayData* append(CVarRef v, bool copy);
   ArrayData* appendRef(CVarRef v, bool copy);
   ArrayData* appendWithRef(CVarRef v, bool copy);
-  ArrayData* append(const ArrayData* elems, ArrayOp op, bool copy);
+  ArrayData* plus(const ArrayData* elems, bool copy);
+  ArrayData* merge(const ArrayData* elems, bool copy);
   ArrayData* pop(Variant& value);
   ArrayData* dequeue(Variant& value);
   ArrayData* prepend(CVarRef v, bool copy);

hphp/runtime/base/array/policy_array.cpp

@@ -735,35 +735,47 @@ void ArrayShell::nextInsertWithRef(const Variant& v) {
   appendNoGrow(k, Variant::NullInit())->setWithRef(v);
 }
 
-ArrayData *ArrayShell::append(const ArrayData *elems, ArrayOp op, bool copy) {
-  APILOG << "(" << elems << ", " << op << ", " << copy << ")";
+ArrayData *ArrayShell::plus(const ArrayData *elems, bool copy) {
+  APILOG << "(" << elems << ", " << copy << ")";
   if (copy) {
-    return ArrayShell::copy()->append(elems, op, false);
+    return ArrayShell::copy()->plus(elems, false);
   }
 
   assert(elems);
-  assert(op == Plus || op == Merge);
   grow(m_size, m_size + 1, m_size * 2 + 1, m_allocMode);
 
   for (ArrayIter it(elems); !it.end(); it.next()) {
     Variant key = it.first();
     const Variant& value = it.secondRef();
-    if (op == Plus) {
-      if (key.isNumeric()) {
-        addValWithRef(key.toInt64(), value);
-      } else {
-        addValWithRef(key.getStringData(), value);
-      }
+    if (key.isNumeric()) {
+      addValWithRef(key.toInt64(), value);
     } else {
-      if (key.isNumeric()) {
-        nextInsertWithRef(value);
-      } else {
-        StringData *s = key.getStringData();
-        Variant *p;
-        // Andrei TODO: make sure this is the right semantics
-        lval(s, p, false, true);
-        p->setWithRef(value);
-      }
+      addValWithRef(key.getStringData(), value);
+    }
+  }
+  return this;
+}
+
+ArrayData *ArrayShell::merge(const ArrayData *elems, bool copy) {
+  APILOG << "(" << elems << ", " << copy << ")";
+  if (copy) {
+    return ArrayShell::copy()->merge(elems, false);
+  }
+
+  assert(elems);
+  grow(m_size, m_size + 1, m_size * 2 + 1, m_allocMode);
+
+  for (ArrayIter it(elems); !it.end(); it.next()) {
+    Variant key = it.first();
+    const Variant& value = it.secondRef();
+    if (key.isNumeric()) {
+      nextInsertWithRef(value);
+    } else {
+      StringData *s = key.getStringData();
+      Variant *p;
+      // Andrei TODO: make sure this is the right semantics
+      lval(s, p, false, true);
+      p->setWithRef(value);
     }
   }
   return this;

hphp/runtime/base/array/policy_array.h

@@ -658,8 +658,8 @@ public:
    * first then append/merge arrays. Return NULL if escalation is not needed,
    * or an escalated array data.
    */
-  virtual ArrayData *append(const ArrayData *elems, ArrayOp op, bool copy)
-    FOLLY_OVERRIDE;
+  virtual ArrayData *plus(const ArrayData *elems, bool copy) FOLLY_OVERRIDE;
+  virtual ArrayData *merge(const ArrayData *elems, bool copy) FOLLY_OVERRIDE;
 
   /**
    * Stack function: pop the last item and return it.

hphp/runtime/base/shared/shared_map.cpp

@@ -179,9 +179,14 @@ ArrayData *SharedMap::appendWithRef(CVarRef v, bool copy) {
   return releaseIfCopied(escalated, escalated->appendWithRef(v, false));
 }
 
-ArrayData *SharedMap::append(const ArrayData *elems, ArrayOp op, bool copy) {
+ArrayData *SharedMap::plus(const ArrayData *elems, bool copy) {
   ArrayData *escalated = SharedMap::escalate();
-  return releaseIfCopied(escalated, escalated->append(elems, op, false));
+  return releaseIfCopied(escalated, escalated->plus(elems, false));
+}
+
+ArrayData *SharedMap::merge(const ArrayData *elems, bool copy) {
+  ArrayData *escalated = SharedMap::escalate();
+  return releaseIfCopied(escalated, escalated->merge(elems, false));
 }
 
 ArrayData *SharedMap::prepend(CVarRef v, bool copy) {

hphp/runtime/base/shared/shared_map.h

@@ -102,7 +102,8 @@ public:
   ArrayData *append(CVarRef v, bool copy);
   ArrayData *appendRef(CVarRef v, bool copy);
   ArrayData *appendWithRef(CVarRef v, bool copy);
-  ArrayData *append(const ArrayData *elems, ArrayOp op, bool copy);
+  ArrayData *plus(const ArrayData *elems, bool copy);
+  ArrayData *merge(const ArrayData *elems, bool copy);
 
   ArrayData *prepend(CVarRef v, bool copy);
 

hphp/runtime/base/type_array.cpp

@@ -84,23 +84,17 @@ Array &Array::operator =  (Variant&& v) {
 }
 
 Array Array::operator+(ArrayData *data) const {
-  return Array(m_px).operator+=(data);
-}
-
-Array Array::operator+(CVarRef var) const {
-  if (var.getType() != KindOfArray) {
-    throw BadArrayMergeException();
-  }
-  return operator+(var.getArrayData());
+  return Array(m_px).plusImpl(data);
 }
 
 Array Array::operator+(CArrRef arr) const {
-  return operator+(arr.m_px);
+  return Array(m_px).plusImpl(arr.m_px);
 }
 
 Array &Array::operator+=(ArrayData *data) {
-  return mergeImpl(data, ArrayData::Plus);
+  return plusImpl(data);
 }
+
 Array &Array::operator+=(CVarRef var) {
   if (var.getType() != KindOfArray) {
     throw BadArrayMergeException();
@@ -109,7 +103,7 @@ Array &Array::operator+=(CVarRef var) {
 }
 
 Array &Array::operator+=(CArrRef arr) {
-  return mergeImpl(arr.m_px, ArrayData::Plus);
+  return plusImpl(arr.m_px);
 }
 
 Array Array::diff(CVarRef array, bool by_key, bool by_value,
@@ -272,22 +266,34 @@ Array Array::diffImpl(CArrRef array, bool by_key, bool by_value, bool match,
 // manipulations
 
 Array &Array::merge(CArrRef arr) {
-  return mergeImpl(arr.m_px, ArrayData::Merge);
+  return mergeImpl(arr.m_px);
 }
 
 HOT_FUNC
-Array &Array::mergeImpl(ArrayData *data, ArrayData::ArrayOp op) {
+Array &Array::plusImpl(ArrayData *data) {
   if (m_px == nullptr || data == nullptr) {
     throw BadArrayMergeException();
   }
   if (!data->empty()) {
-    if (op != ArrayData::Merge && m_px->empty()) {
+    if (m_px->empty()) {
       ArrayBase::operator=(data);
-    } else if (m_px != data || op == ArrayData::Merge) {
-      ArrayData *escalated = m_px->append(data, op, m_px->getCount() > 1);
+    } else if (m_px != data) {
+      ArrayData *escalated = m_px->plus(data, m_px->getCount() > 1);
       if (escalated != m_px) ArrayBase::operator=(escalated);
     }
-  } else if (op == ArrayData::Merge) {
+  }
+  return *this;
+}
+
+HOT_FUNC
+Array &Array::mergeImpl(ArrayData *data) {
+  if (m_px == nullptr || data == nullptr) {
+    throw BadArrayMergeException();
+  }
+  if (!data->empty()) {
+    ArrayData *escalated = m_px->merge(data, m_px->getCount() > 1);
+    if (escalated != m_px) ArrayBase::operator=(escalated);
+  } else {
     m_px->renumber();
   }
   return *this;

hphp/runtime/base/type_array.h

@@ -131,7 +131,7 @@ class Array : protected SmartPtr<ArrayData> {
   Array &operator =  (CVarRef v);
   Array  operator +  (ArrayData *data) const;
   Array  operator +  (CArrRef v) const;
-  Array  operator +  (CVarRef v) const;
+  Array  operator +  (CVarRef v) const = delete;
   Array &operator += (ArrayData *data);
   Array &operator += (CArrRef v);
   Array &operator += (CVarRef v);
@@ -460,7 +460,8 @@ class Array : protected SmartPtr<ArrayData> {
   // helpers
   bool compare(CArrRef v2) const;
   static int CompareAsStrings(CVarRef v1, CVarRef v2, const void *data);
-  Array &mergeImpl(ArrayData *data, ArrayData::ArrayOp op);
+  Array &plusImpl(ArrayData *data);
+  Array &mergeImpl(ArrayData *data);
   Array diffImpl(CArrRef array, bool by_key, bool by_value, bool match,
                  PFUNC_CMP key_cmp_function, const void *key_data,
                  PFUNC_CMP value_cmp_function, const void *value_data) const;

hphp/runtime/base/type_variant.cpp

@@ -712,13 +712,11 @@ Variant &Variant::operator+=(CVarRef var) {
       set(arr2);
       return *this;
     }
-    ArrayData *escalated = arr1->append(arr2, ArrayData::Plus,
-                                        arr1->getCount() > 1);
+    ArrayData *escalated = arr1->plus(arr2, arr1->getCount() > 1);
     if (escalated != arr1) set(escalated);
     return *this;
-  } else if (na) {
-    throw BadArrayMergeException();
   }
+  if (na) throw BadArrayMergeException();
   if (isDouble() || var.isDouble()) {
     set(toDouble() + var.toDouble());
     return *this;

hphp/runtime/vm/jit/translator-runtime.cpp

@@ -62,8 +62,7 @@ ArrayData* array_add(ArrayData* a1, ArrayData* a2) {
       return a2;
     }
     if (a1 != a2) {
-      ArrayData *escalated = a1->append(a2, ArrayData::Plus,
-                                        a1->getCount() > 1);
+      ArrayData *escalated = a1->plus(a2, a1->getCount() > 1);
       if (escalated != a1) {
         escalated->incRefCount();
         decRefArr(a2);

hphp/runtime/vm/name_value_table_wrapper.cpp

@@ -161,10 +161,12 @@ ArrayData* NameValueTableWrapper::appendWithRef(CVarRef v, bool copy) {
   throw NotImplementedException("appendWithRef on $GLOBALS");
 }
 
-ArrayData* NameValueTableWrapper::append(const ArrayData* elems,
-                                         ArrayOp op,
-                                         bool copy) {
-  throw NotImplementedException("append on $GLOBALS");
+ArrayData* NameValueTableWrapper::plus(const ArrayData* elems, bool copy) {
+  throw NotImplementedException("plus on $GLOBALS");
+}
+
+ArrayData* NameValueTableWrapper::merge(const ArrayData* elems, bool copy) {
+  throw NotImplementedException("merge on $GLOBALS");
 }
 
 ArrayData* NameValueTableWrapper::prepend(CVarRef v, bool copy) {

hphp/runtime/vm/name_value_table_wrapper.h

@@ -109,7 +109,8 @@ public: // ArrayData implementation
   virtual ArrayData* appendRef(CVarRef v, bool copy);
   virtual ArrayData* appendWithRef(CVarRef v, bool copy);
 
-  virtual ArrayData* append(const ArrayData* elems, ArrayOp op, bool copy);
+  virtual ArrayData* plus(const ArrayData* elems, bool copy);
+  virtual ArrayData* merge(const ArrayData* elems, bool copy);
 
   virtual ArrayData* prepend(CVarRef v, bool copy);