new considered harmful

I'm committing my work on HphpArray in bite-sized pieces for easier review. This piece replaces calls to NEW with a factory method. There are many problems with operator new, starting with the fact that the allocator cannot communicate properly with the constructor.

The newly introduced factory method should return ArrayData but that causes many issues right now, so I left that step to a future diff.

hphp/compiler/analysis/emitter.cpp

@@ -6809,7 +6809,7 @@ void EmitterVisitor::initScalar(TypedValue& tvVal, ExpressionPtr val) {
     case Expression::KindOfUnaryOpExpression: {
       UnaryOpExpressionPtr u(static_pointer_cast<UnaryOpExpression>(val));
       if (u->getOp() == T_ARRAY) {
-        auto a = NEW(HphpArray)(0);
+        auto a = ArrayData::Make(0);
         a->incRefCount();
         m_staticArrays.push_back(a);
 

hphp/runtime/base/array/array_data.h

@@ -27,6 +27,7 @@ namespace HPHP {
 
 class SharedVariant;
 struct TypedValue;
+class HphpArray;
 
 /**
  * Base class/interface for all types of specialized array data.
@@ -64,6 +65,9 @@ class ArrayData : public Countable {
     m_nonsmart(nonsmart) {
   }
 
+  static HphpArray* Make(uint capacity);
+  static HphpArray* Make(uint size, const TypedValue*);
+
   virtual ~ArrayData() {
     // If there are any strong iterators pointing to this array, they need
     // to be invalidated.

hphp/runtime/base/array/array_init.cpp

@@ -30,18 +30,18 @@ ArrayInit::ArrayInit(ssize_t n) {
     // Force compilation of ArrayShell
     m_data = NEW(ArrayShell)(n);
   } else {
-    m_data = NEW(HphpArray)(n);
+    m_data = ArrayData::Make(n);
   }
 }
 
 HOT_FUNC
 ArrayData *ArrayInit::CreateVector(ssize_t n) {
-  return NEW(HphpArray)(n);
+  return ArrayData::Make(n);
 }
 
 HOT_FUNC
 ArrayData *ArrayInit::CreateMap(ssize_t n) {
-  return NEW(HphpArray)(n);
+  return ArrayData::Make(n);
 }
 
 ArrayData *ArrayInit::CreateParams(int count, ...) {

hphp/runtime/base/array/hphp_array.cpp

@@ -154,8 +154,8 @@ HphpArray::HphpArray(EmptyMode) : ArrayData(kHphpArray),
 }
 
 // Empty constructor for internal use by nonSmartCopy() and copyImpl()
-HphpArray::HphpArray(CopyMode mode) :
-  ArrayData(kHphpArray, /*nonsmart*/ mode == kNonSmartCopy) {
+HphpArray::HphpArray(AllocationPolicy mode) :
+    ArrayData(kHphpArray, /*nonsmart*/ mode == AllocationPolicy::nonSmart) {
 }
 
 HOT_FUNC_VM
@@ -1446,11 +1446,11 @@ inline ALWAYS_INLINE HphpArray* HphpArray::copyImpl(HphpArray* target) const {
 }
 
 NEVER_INLINE ArrayData* HphpArray::nonSmartCopy() const {
-  return copyImpl(new HphpArray(kNonSmartCopy));
+  return copyImpl(new HphpArray(AllocationPolicy::nonSmart));
 }
 
 NEVER_INLINE HphpArray* HphpArray::copyImpl() const {
-  return copyImpl(NEW(HphpArray)(kSmartCopy));
+  return copyImpl(NEW(HphpArray)(AllocationPolicy::smart));
 }
 
 ArrayData* HphpArray::append(CVarRef v, bool copy) {

hphp/runtime/base/array/hphp_array.h

@@ -28,7 +28,7 @@ namespace HPHP {
 class ArrayInit;
 
 class HphpArray : public ArrayData {
-  enum CopyMode { kSmartCopy, kNonSmartCopy };
+  enum class AllocationPolicy { smart, nonSmart };
   enum SortFlavor { IntegerSort, StringSort, GenericSort };
 public:
   friend class ArrayInit;
@@ -46,11 +46,11 @@ public:
 
 private:
   // for copy-on-write escalation
-  HphpArray(CopyMode);
+  explicit HphpArray(AllocationPolicy);
 
 public:
   // Create an empty array with enough capacity for nSize elements.
-  HphpArray(uint nSize);
+  explicit HphpArray(uint nSize);
 
   // Create and initialize an array with size elements, populated by
   // moving (without refcounting) and reversing vals.
@@ -474,7 +474,7 @@ private:
 
 private:
   enum EmptyMode { StaticEmptyArray };
-  HphpArray(EmptyMode);
+  explicit HphpArray(EmptyMode);
   // static singleton empty array.  Not a subclass because we want a fast
   // isHphpArray implementation; HphpArray should be effectively final.
   static HphpArray s_theEmptyArray;
@@ -546,6 +546,16 @@ ArrayData* array_add(ArrayData* a1, ArrayData* a2);
 
 //=============================================================================
 
+// inline for performance reasons
+
+inline HphpArray* ArrayData::Make(uint capacity) {
+  return NEW(HphpArray)(capacity);
+}
+
+inline HphpArray* ArrayData::Make(uint size, const TypedValue* data) {
+  return NEW(HphpArray)(size, data);
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 }
 

hphp/runtime/base/array/policy_array.cpp

@@ -590,7 +590,7 @@ CVarRef ArrayShell::endRef() {
 }
 
 HphpArray* ArrayShell::toHphpArray() const {
-  auto result = NEW(HphpArray)(m_size);
+  auto result = ArrayData::Make(m_size);
   FOR_EACH_RANGE (i, 0, m_size) {
     if (hasStrKey(toPos(i))) {
       result->add(key(toPos(i)).getStringData(), val(toPos(i)), false);

hphp/runtime/base/execution_context.h

@@ -546,7 +546,7 @@ public:
       return it->second;
     }
 
-    HphpArray* array = NEW(HphpArray)(f->numStaticLocals());
+    auto array = ArrayData::Make(f->numStaticLocals());
     array->incRefCount();
     return m_funcStaticCtx[f] = array;
   }

hphp/runtime/base/object_data.cpp

@@ -389,7 +389,7 @@ Array ObjectData::o_toIterArray(CStrRef context,
                                 bool getRef /* = false */) {
   size_t size = m_cls->m_declPropNumAccessible +
                 (o_properties.get() ? o_properties.get()->size() : 0);
-  HphpArray* retval = NEW(HphpArray)(size);
+  auto retval = ArrayData::Make(size);
   Class* ctx = nullptr;
   if (!context.empty()) {
     ctx = Unit::lookupClass(context.get());

hphp/runtime/ext/ext_class.cpp

@@ -84,7 +84,7 @@ Array f_get_class_methods(CVarRef class_or_object) {
   if (!cls) return Array();
   VMRegAnchor _;
 
-  HphpArray* retVal = NEW(HphpArray)(cls->numMethods());
+  auto retVal = ArrayData::Make(cls->numMethods());
   cls->getMethodNames(arGetContextClassFromBuiltin(g_vmContext->getFP()),
                       retVal);
   return Array(retVal).keys();
@@ -93,11 +93,11 @@ Array f_get_class_methods(CVarRef class_or_object) {
 Array vm_get_class_constants(CStrRef className) {
   HPHP::Class* cls = HPHP::Unit::loadClass(className.get());
   if (cls == NULL) {
-    return NEW(HphpArray)(0);
+    return ArrayData::Make(0);
   }
 
   size_t numConstants = cls->numConstants();
-  HphpArray* retVal = NEW(HphpArray)(numConstants);
+  auto retVal = ArrayData::Make(numConstants);
   const Class::Const* consts = cls->constants();
   for (size_t i = 0; i < numConstants; i++) {
     // Note: hphpc doesn't include inherited constants in
@@ -145,7 +145,7 @@ Array vm_get_class_vars(CStrRef className) {
   CallerFrame cf;
   HPHP::Class* ctx = arGetContextClass(cf());
 
-  HphpArray* ret = NEW(HphpArray)(numDeclProps + numSProps);
+  auto ret = ArrayData::Make(numDeclProps + numSProps);
 
   for (size_t i = 0; i < numDeclProps; ++i) {
     StringData* name = const_cast<StringData*>(propInfo[i].m_name);

hphp/runtime/ext/ext_closure.cpp

@@ -87,7 +87,7 @@ bool c_Closure::php_sleep(Variant &ret) {
 
 HphpArray* c_Closure::getStaticLocals() {
   if (m_VMStatics.get() == NULL) {
-    m_VMStatics = NEW(HphpArray)(1);
+    m_VMStatics = ArrayData::Make(1);
   }
   return m_VMStatics.get();
 }

hphp/runtime/ext/ext_continuation.cpp

@@ -171,7 +171,7 @@ Variant c_Continuation::t___clone() {
 
 HphpArray* c_Continuation::getStaticLocals() {
   if (m_VMStatics.get() == NULL) {
-    m_VMStatics = NEW(HphpArray)(1);
+    m_VMStatics = ArrayData::Make(1);
   }
   return m_VMStatics.get();
 }

hphp/runtime/ext/ext_function.cpp

@@ -347,7 +347,7 @@ Array hhvm_get_frame_args(const ActRec* ar) {
   }
   int numParams = ar->m_func->numParams();
   int numArgs = ar->numArgs();
-  HphpArray* retval = NEW(HphpArray)(numArgs);
+  auto retval = ArrayData::Make(numArgs);
 
   TypedValue* local = (TypedValue*)(uintptr_t(ar) - sizeof(TypedValue));
   for (int i = 0; i < numArgs; ++i) {

hphp/runtime/vm/bytecode.cpp

@@ -3831,7 +3831,7 @@ inline void OPTBLD_INLINE VMExecutionContext::iopArray(PC& pc) {
 inline void OPTBLD_INLINE VMExecutionContext::iopNewArray(PC& pc) {
   NEXT();
   // Clever sizing avoids extra work in HphpArray construction.
-  ArrayData* arr = NEW(HphpArray)(size_t(3U) << (HphpArray::MinLgTableSize-2));
+  auto arr = ArrayData::Make(size_t(3U) << (HphpArray::MinLgTableSize-2));
   m_stack.pushArray(arr);
 }
 
@@ -3839,7 +3839,7 @@ inline void OPTBLD_INLINE VMExecutionContext::iopNewTuple(PC& pc) {
   NEXT();
   DECODE_IVA(n);
   // This constructor moves values, no inc/decref is necessary.
-  HphpArray* arr = NEW(HphpArray)(n, m_stack.topC());
+  HphpArray* arr = ArrayData::Make(n, m_stack.topC());
   m_stack.ndiscard(n);
   m_stack.pushArray(arr);
 }

hphp/runtime/vm/class.cpp

@@ -823,7 +823,7 @@ Class::PropInitVec* Class::initPropsImpl() const {
   propArr.incRefCount();
 
   // Insert propArr and sentinel into the args array, transferring ownership.
-  ArrayData* args = NEW(HphpArray)(2);
+  ArrayData* args = ArrayData::Make(2);
   args->incRefCount();
   {
     // the first argument is byRef. Make a reference,
@@ -1023,7 +1023,7 @@ TypedValue* Class::initSPropsImpl() const {
     NameValueTableWrapper nvtWrapper(&*nvt);
     nvtWrapper.incRefCount();
 
-    ArrayData* args = NEW(HphpArray)(1);
+    ArrayData* args = ArrayData::Make(1);
     args->incRefCount();
     try {
       {
@@ -1120,7 +1120,7 @@ TypedValue Class::getStaticPropInitVal(const SProp& prop) {
 
 HphpArray* Class::initClsCnsData() const {
   Slot nConstants = m_constants.size();
-  HphpArray* constants = NEW(HphpArray)(nConstants);
+  HphpArray* constants = ArrayData::Make(nConstants);
   constants->incRefCount();
 
   if (m_parent.get() != nullptr) {

hphp/runtime/vm/event_hook.cpp

@@ -109,7 +109,7 @@ void EventHook::RunUserProfiler(const ActRec* ar, int mode) {
 static Array get_frame_args_with_ref(const ActRec* ar) {
   int numParams = ar->m_func->numParams();
   int numArgs = ar->numArgs();
-  HphpArray* retval = NEW(HphpArray)(numArgs);
+  HphpArray* retval = ArrayData::Make(numArgs);
 
   TypedValue* local = (TypedValue*)(uintptr_t(ar) - sizeof(TypedValue));
   for (int i = 0; i < numArgs; ++i) {

hphp/runtime/vm/instance.cpp

@@ -122,7 +122,7 @@ Object Instance::FromArray(ArrayData *properties) {
 
 void Instance::initDynProps(int numDynamic /* = 0 */) {
   // Create o_properties with room for numDynamic
-  o_properties.asArray() = NEW(HphpArray)(numDynamic);
+  o_properties.asArray() = ArrayData::Make(numDynamic);
 }
 
 Slot Instance::declPropInd(TypedValue* prop) const {

hphp/runtime/vm/runtime.cpp

@@ -67,14 +67,14 @@ void print_boolean(bool val) {
 
 HOT_FUNC_VM
 ArrayData* new_array(int capacity) {
-  ArrayData *a = NEW(HphpArray)(capacity);
+  ArrayData *a = ArrayData::Make(capacity);
   a->incRefCount();
   TRACE(2, "newArrayHelper: capacity %d\n", capacity);
   return a;
 }
 
 ArrayData* new_tuple(int n, const TypedValue* values) {
-  HphpArray* a = NEW(HphpArray)(n, values);
+  auto a = ArrayData::Make(n, values);
   a->incRefCount();
   TRACE(2, "new_tuple: size %d\n", n);
   return a;
@@ -342,7 +342,7 @@ Unit* compile_string(const char* s, size_t sz) {
 
 // Returned array has refcount zero! Caller must refcount.
 HphpArray* pack_args_into_array(ActRec* ar, int nargs) {
-  HphpArray* argArray = NEW(HphpArray)(nargs);
+  HphpArray* argArray = ArrayData::Make(nargs);
   for (int i = 0; i < nargs; ++i) {
     TypedValue* tv = (TypedValue*)(ar) - (i+1);
     argArray->HphpArray::appendWithRef(tvAsCVarRef(tv), false);
@@ -352,7 +352,7 @@ HphpArray* pack_args_into_array(ActRec* ar, int nargs) {
     return argArray;
   }
   // This is a magic call, so we need to shuffle the args
-  HphpArray* magicArgs = NEW(HphpArray)(2);
+  HphpArray* magicArgs = ArrayData::Make(2);
   magicArgs->append(ar->getInvName(), false);
   magicArgs->append(argArray, false);
   return magicArgs;

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

@@ -1646,7 +1646,7 @@ TranslatorX64::shuffleArgsForMagicCall(ActRec* ar) {
   int nargs = ar->numArgs();
   // We need to make an array containing all the arguments passed by the
   // caller and put it where the second argument is
-  HphpArray* argArray = NEW(HphpArray)(nargs);
+  HphpArray* argArray = ArrayData::Make(nargs);
   argArray->incRefCount();
   for (int i = 0; i < nargs; ++i) {
     TypedValue* tv =

hphp/runtime/vm/unit.cpp

@@ -929,7 +929,7 @@ bool Unit::defCns(const StringData* cnsName, const TypedValue* value,
        * static string. Not worth presizing or otherwise
        * optimizing for.
        */
-      TargetCache::s_constants = NEW(HphpArray)(1);
+      TargetCache::s_constants = ArrayData::Make(1);
       TargetCache::s_constants->incRefCount();
     }
     if (TargetCache::s_constants->nvInsert(