diff --git a/hphp/runtime/base/array/array_data.h b/hphp/runtime/base/array/array_data.h index a1915bbf4..737f005db 100644 --- a/hphp/runtime/base/array/array_data.h +++ b/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. diff --git a/hphp/runtime/base/array/hphp_array.cpp b/hphp/runtime/base/array/hphp_array.cpp index 393b2f72c..264b7288e 100644 --- a/hphp/runtime/base/array/hphp_array.cpp +++ b/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; diff --git a/hphp/runtime/base/array/hphp_array.h b/hphp/runtime/base/array/hphp_array.h index e6aaa0872..31254c922 100644 --- a/hphp/runtime/base/array/hphp_array.h +++ b/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); diff --git a/hphp/runtime/base/array/policy_array.cpp b/hphp/runtime/base/array/policy_array.cpp index a6d2ec182..4276e388e 100644 --- a/hphp/runtime/base/array/policy_array.cpp +++ b/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; diff --git a/hphp/runtime/base/array/policy_array.h b/hphp/runtime/base/array/policy_array.h index 1622fcd3e..29acc3c2c 100644 --- a/hphp/runtime/base/array/policy_array.h +++ b/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. diff --git a/hphp/runtime/base/shared/shared_map.cpp b/hphp/runtime/base/shared/shared_map.cpp index f377e0523..06dcaf941 100644 --- a/hphp/runtime/base/shared/shared_map.cpp +++ b/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) { diff --git a/hphp/runtime/base/shared/shared_map.h b/hphp/runtime/base/shared/shared_map.h index 5723f1af8..15ea12988 100644 --- a/hphp/runtime/base/shared/shared_map.h +++ b/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); diff --git a/hphp/runtime/base/type_array.cpp b/hphp/runtime/base/type_array.cpp index a374fed80..75d295fba 100644 --- a/hphp/runtime/base/type_array.cpp +++ b/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; diff --git a/hphp/runtime/base/type_array.h b/hphp/runtime/base/type_array.h index a926f4c43..49931db8e 100644 --- a/hphp/runtime/base/type_array.h +++ b/hphp/runtime/base/type_array.h @@ -131,7 +131,7 @@ class Array : protected SmartPtr { 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 { // 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; diff --git a/hphp/runtime/base/type_variant.cpp b/hphp/runtime/base/type_variant.cpp index 60a7cd067..0b99da5c9 100644 --- a/hphp/runtime/base/type_variant.cpp +++ b/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; diff --git a/hphp/runtime/vm/jit/translator-runtime.cpp b/hphp/runtime/vm/jit/translator-runtime.cpp index 36998ddb0..7a22d4929 100644 --- a/hphp/runtime/vm/jit/translator-runtime.cpp +++ b/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); diff --git a/hphp/runtime/vm/name_value_table_wrapper.cpp b/hphp/runtime/vm/name_value_table_wrapper.cpp index d417b4104..7581b88fa 100644 --- a/hphp/runtime/vm/name_value_table_wrapper.cpp +++ b/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) { diff --git a/hphp/runtime/vm/name_value_table_wrapper.h b/hphp/runtime/vm/name_value_table_wrapper.h index 76b31dda7..008570081 100644 --- a/hphp/runtime/vm/name_value_table_wrapper.h +++ b/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);