Eliminate CreateLvalPtr and GetLvalPtr

HphpArray::CreateLvalPtr does the same thing as LvalStr().  SharedMap
and NameValueTableWrapper::CreateLvalPtr just fatal, and aren't called
anyway.  GetLvalPtr is only called from one place which suppresses COW
and thus can also be done with nvGet, leaving getLvalPtr dead.

hphp/runtime/base/array_data-defs.h

@@ -78,17 +78,6 @@ inline ArrayData* ArrayData::lval(CVarRef k, Variant *&ret, bool copy) {
                         : lval(getStringKey(cell), ret, copy);
 }
 
-inline
-ArrayData *ArrayData::createLvalPtr(CStrRef k, Variant *&ret, bool copy) {
-  assert(IsValidKey(k));
-  return createLvalPtr(k.get(), ret, copy);
-}
-
-inline ArrayData *ArrayData::getLvalPtr(CStrRef k, Variant *&ret, bool copy) {
-  assert(IsValidKey(k));
-  return getLvalPtr(k.get(), ret, copy);
-}
-
 inline ArrayData* ArrayData::set(CStrRef k, CVarRef v, bool copy) {
   assert(IsValidKey(k));
   return set(k.get(), v, copy);
@@ -237,16 +226,6 @@ inline ArrayData* ArrayData::lvalNew(Variant*& ret, bool copy) {
   return g_array_funcs.lvalNew[m_kind](this, ret, copy);
 }
 
-inline ArrayData* ArrayData::createLvalPtr(StringData* k, Variant*& ret,
-                                           bool copy) {
-  return g_array_funcs.createLvalPtr[m_kind](this, k, ret, copy);
-}
-
-inline ArrayData* ArrayData::getLvalPtr(StringData* k, Variant*& ret,
-                                        bool copy) {
-  return g_array_funcs.getLvalPtr[m_kind](this, k, ret, copy);
-}
-
 inline ArrayData* ArrayData::setRef(int64_t k, CVarRef v, bool copy) {
   return g_array_funcs.setRefInt[m_kind](this, k, v, copy);
 }

hphp/runtime/base/array_data.cpp

@@ -146,16 +146,6 @@ extern const ArrayFunctions g_array_funcs = {
     &SharedMap::LvalNew,
     &NameValueTableWrapper::LvalNew,
     &PolicyArray::LvalNew },
-  // createLvalPtr
-  { &HphpArray::CreateLvalPtrVec, &HphpArray::CreateLvalPtr,
-    &ArrayData::CreateLvalPtr, // fatal
-    &ArrayData::CreateLvalPtr, // fatal
-    &PolicyArray::CreateLvalPtr },
-  // getLvalPtr
-  { &HphpArray::GetLvalPtrVec, &HphpArray::GetLvalPtr,
-    &ArrayData::GetLvalPtr, // fatal
-    &ArrayData::GetLvalPtr, // fatal
-    &PolicyArray::GetLvalPtr },
   // setRefInt
   { &HphpArray::SetRefIntVec, &HphpArray::SetRefInt,
     &SharedMap::SetRefInt,
@@ -448,16 +438,6 @@ bool ArrayData::equal(const ArrayData *v2, bool strict) const {
   return true;
 }
 
-ArrayData *ArrayData::CreateLvalPtr(ArrayData* ad, StringData* k,
-                                    Variant *&ret, bool copy) {
-  throw FatalErrorException("Unimplemented ArrayData::createLvalPtr");
-}
-
-ArrayData *ArrayData::GetLvalPtr(ArrayData* ad, StringData* k,
-                                 Variant *&ret, bool copy) {
-  throw FatalErrorException("Unimplemented ArrayData::getLvalPtr");
-}
-
 ///////////////////////////////////////////////////////////////////////////////
 // stack and queue operations
 

hphp/runtime/base/array_data.h

@@ -260,18 +260,6 @@ public:
    */
   ArrayData *lvalNew(Variant *&ret, bool copy);
 
-  /**
-   * Helper functions used for getting a reference to elements of
-   * the dynamic property array in ObjectData or the local cache array
-   * in ShardMap.
-   */
-  ArrayData *createLvalPtr(StringData* k, Variant *&ret, bool copy);
-  ArrayData *getLvalPtr(StringData* k, Variant *&ret, bool copy);
-  static ArrayData *CreateLvalPtr(ArrayData* ad, StringData* k,
-                                  Variant *&ret, bool copy);
-  static ArrayData *GetLvalPtr(ArrayData* ad, StringData* k,
-                               Variant *&ret, bool copy);
-
   /**
    * Setting a value at specified key. If "copy" is true, make a copy first
    * then set the value. Return this if escalation is not needed, or an
@@ -319,8 +307,6 @@ public:
   CVarRef get(CVarRef k, bool error = false) const;
   ArrayData *lval(CStrRef k, Variant *&ret, bool copy);
   ArrayData *lval(CVarRef k, Variant *&ret, bool copy);
-  ArrayData *createLvalPtr(CStrRef k, Variant *&ret, bool copy);
-  ArrayData *getLvalPtr(CStrRef k, Variant *&ret, bool copy);
   ArrayData *set(CStrRef k, CVarRef v, bool copy);
   ArrayData *set(CVarRef k, CVarRef v, bool copy);
   ArrayData *set(const StringData*, CVarRef, bool) = delete;
@@ -572,10 +558,6 @@ struct ArrayFunctions {
   ArrayData* (*lvalStr[NK])(ArrayData*, StringData* k, Variant*& ret,
                             bool copy);
   ArrayData* (*lvalNew[NK])(ArrayData*, Variant *&ret, bool copy);
-  ArrayData* (*createLvalPtr[NK])(ArrayData*, StringData* k, Variant *&ret,
-                                  bool copy);
-  ArrayData* (*getLvalPtr[NK])(ArrayData*, StringData* k, Variant *&ret,
-                               bool copy);
   ArrayData* (*setRefInt[NK])(ArrayData*, int64_t k, CVarRef v, bool copy);
   ArrayData* (*setRefStr[NK])(ArrayData*, StringData* k, CVarRef v, bool copy);
   ArrayData* (*addInt[NK])(ArrayData*, int64_t k, CVarRef v, bool copy);

hphp/runtime/base/hphp_array.cpp

@@ -933,21 +933,7 @@ ArrayData* HphpArray::nextInsertWithRef(CVarRef data) {
 }
 
 ArrayData* HphpArray::addLvalImpl(int64_t ki, Variant** pDest) {
-  assert(pDest != nullptr);
-  if (isVector()) {
-    if (size_t(ki) < m_size) {
-      *pDest = &tvAsVariant(&m_data[ki].data);
-      return this;
-    }
-    if (size_t(ki) == m_size) {
-      auto& tv = allocNextElm(ki);
-      tvWriteNull(&tv);
-      *pDest = &(tvAsVariant(&tv));
-      return this;
-    }
-    vectorToGeneric();
-    // todo t2606310: we know key is new.  use add/findForNewInsert
-  }
+  assert(pDest && !isVector());
   ElmInd* ei = findForInsert(ki);
   if (validElmInd(*ei)) {
     *pDest = &tvAsVariant(&m_data[*ei].data);
@@ -965,11 +951,7 @@ ArrayData* HphpArray::addLvalImpl(int64_t ki, Variant** pDest) {
 
 ArrayData* HphpArray::addLvalImpl(StringData* key, strhash_t h,
                                   Variant** pDest) {
-  assert(key != nullptr && pDest != nullptr);
-  if (isVector()) {
-    vectorToGeneric();
-    // todo t2606310: now we know key is new. use add/findForNewInsert
-  }
+  assert(key && pDest && !isVector());
   ElmInd* ei = findForInsert(key, h);
   if (validElmInd(*ei)) {
     Elm* e = &m_data[*ei];
@@ -1112,7 +1094,18 @@ ArrayData* HphpArray::LvalIntVec(ArrayData* ad, int64_t k, Variant*& ret,
                                  bool copy) {
   auto a = asVector(ad);
   if (copy) a = a->copyVec();
-  return a->addLvalImpl(k, &ret);
+  if (size_t(k) < a->m_size) {
+    ret = &tvAsVariant(&a->m_data[k].data);
+    return a;
+  }
+  if (size_t(k) == a->m_size) {
+    auto& tv = a->allocNextElm(k);
+    tvWriteNull(&tv);
+    ret = &(tvAsVariant(&tv));
+    return a;
+  }
+  // todo t2606310: we know key is new.  use add/findForNewInsert
+  return a->vectorToGeneric()->addLvalImpl(k, &ret);
 }
 
 ArrayData* HphpArray::LvalInt(ArrayData* ad, int64_t k, Variant*& ret,
@@ -1126,7 +1119,7 @@ ArrayData* HphpArray::LvalStrVec(ArrayData* ad, StringData* key, Variant*& ret,
                                  bool copy) {
   auto a = asVector(ad);
   if (copy) a = a->copyVec();
-  return a->addLvalImpl(key, key->hash(), &ret);
+  return a->vectorToGeneric()->addLvalImpl(key, key->hash(), &ret);
 }
 
 ArrayData* HphpArray::LvalStr(ArrayData* ad, StringData* key, Variant*& ret,
@@ -1136,42 +1129,6 @@ ArrayData* HphpArray::LvalStr(ArrayData* ad, StringData* key, Variant*& ret,
   return a->addLvalImpl(key, key->hash(), &ret);
 }
 
-ArrayData *HphpArray::CreateLvalPtrVec(ArrayData* ad, StringData* key,
-                                       Variant*& ret, bool copy) {
-  auto a = asVector(ad);
-  if (copy) a = a->copyVec();
-  return a->vectorToGeneric()->addLvalImpl(key, key->hash(), &ret);
-  // todo: we know the key can't exist; use specialized addLvalImpl
-}
-
-ArrayData *HphpArray::CreateLvalPtr(ArrayData* ad, StringData* key,
-                                    Variant*& ret, bool copy) {
-  auto a = asGeneric(ad);
-  if (copy) a = a->copyGeneric();
-  return a->addLvalImpl(key, key->hash(), &ret);
-}
-
-ArrayData *HphpArray::GetLvalPtrVec(ArrayData* ad, StringData* key,
-                                    Variant*& ret, bool copy) {
-  auto a = asVector(ad);
-  if (copy) a = a->copyVec();
-  // todo: we didn't have to copy since we didn't mutate.  Or, should
-  // we have just escalate to generic anyway?  and, why isn't this
-  // method const?
-  ret = nullptr;
-  return a;
-}
-
-ArrayData *HphpArray::GetLvalPtr(ArrayData* ad, StringData* key,
-                                 Variant*& ret, bool copy) {
-  auto a = asGeneric(ad);
-  if (copy) a = a->copyGeneric();
-  auto pos = a->find(key, key->hash());
-  ret = pos != ElmIndEmpty ? &tvAsVariant(&a->m_data[pos].data) :
-        nullptr;
-  return a;
-}
-
 ArrayData* HphpArray::LvalNewVec(ArrayData* ad, Variant*& ret, bool copy) {
   auto a = asVector(ad);
   if (copy) a = a->copyVec();

hphp/runtime/base/hphp_array.h

@@ -92,8 +92,6 @@ public:
   using ArrayData::exists;
   using ArrayData::lval;
   using ArrayData::lvalNew;
-  using ArrayData::createLvalPtr;
-  using ArrayData::getLvalPtr;
   using ArrayData::set;
   using ArrayData::setRef;
   using ArrayData::add;
@@ -130,16 +128,6 @@ public:
   static ArrayData* LvalNew(ArrayData*, Variant*& ret, bool copy);
   static ArrayData* LvalNewVec(ArrayData*, Variant*& ret, bool copy);
 
-  // overrides ArrayData
-  static ArrayData* CreateLvalPtr(ArrayData*, StringData* k, Variant*& ret,
-                                  bool copy);
-  static ArrayData* GetLvalPtr(ArrayData*, StringData* k, Variant*& ret,
-                               bool copy);
-  static ArrayData* CreateLvalPtrVec(ArrayData*, StringData* k, Variant*& ret,
-                                     bool copy);
-  static ArrayData* GetLvalPtrVec(ArrayData*, StringData* k, Variant*& ret,
-                                  bool copy);
-
   // implements ArrayData
   static ArrayData* SetIntVec(ArrayData*, int64_t k, CVarRef v, bool copy);
   static ArrayData* SetStrVec(ArrayData*, StringData* k, CVarRef v, bool copy);

hphp/runtime/base/object_data.cpp

@@ -223,7 +223,7 @@ Variant* ObjectData::o_realProp(CStrRef propName, int flags,
     if (!o_properties.get()) {
       thiz->initDynProps();
     }
-    o_properties.get()->createLvalPtr(propName, *(Variant**)(&ret), false);
+    o_properties.get()->lval(propName, *(Variant**)(&ret), false);
     return (Variant*)ret;
   }
 
@@ -1020,8 +1020,8 @@ void ObjectData::propImpl(TypedValue*& retval, TypedValue& tvRef,
         if (o_properties.get() == nullptr) {
           initDynProps();
         }
-        o_properties.get()->createLvalPtr(*(const String*)&key,
-                                          *(Variant**)(&retval), false);
+        o_properties.get()->lval(*(const String*)&key,
+                                 *(Variant**)(&retval), false);
       } else {
         retval = (TypedValue*)&init_null_variant;
       }
@@ -1186,8 +1186,8 @@ TypedValue* ObjectData::setOpProp(TypedValue& tvRef, Class* ctx,
     if (o_properties.get() == nullptr) {
       initDynProps();
     }
-    o_properties.get()->createLvalPtr(*(const String*)&key,
-                                      *(Variant**)(&propVal), false);
+    o_properties.get()->lval(*(const String*)&key,
+                             *(Variant**)(&propVal), false);
     // don't write propVal->_count because it holds data
     // owned by the HphpArray
     propVal->m_type = KindOfNull;
@@ -1201,8 +1201,8 @@ TypedValue* ObjectData::setOpProp(TypedValue& tvRef, Class* ctx,
     if (o_properties.get() == nullptr) {
       initDynProps();
     }
-    o_properties.get()->createLvalPtr(*(const String*)&key,
-                                      *(Variant**)(&propVal), false);
+    o_properties.get()->lval(*(const String*)&key,
+                             *(Variant**)(&propVal), false);
     // don't write propVal->_count because it holds data
     // owned by the HphpArray
     propVal->m_data.num = tvResult.m_data.num;
@@ -1259,8 +1259,8 @@ void ObjectData::incDecPropImpl(TypedValue& tvRef, Class* ctx,
     if (o_properties.get() == nullptr) {
       initDynProps();
     }
-    o_properties.get()->createLvalPtr(*(const String*)&key,
-                                      *(Variant**)(&propVal), false);
+    o_properties.get()->lval(*(const String*)&key,
+                             *(Variant**)(&propVal), false);
     // don't write propVal->_count because it holds data
     // owned by the HphpArray
     propVal->m_type = KindOfNull;
@@ -1274,8 +1274,8 @@ void ObjectData::incDecPropImpl(TypedValue& tvRef, Class* ctx,
     if (o_properties.get() == nullptr) {
       initDynProps();
     }
-    o_properties.get()->createLvalPtr(*(const String*)&key,
-                                      *(Variant**)(&propVal), false);
+    o_properties.get()->lval(*(const String*)&key,
+                             *(Variant**)(&propVal), false);
     // don't write propVal->_count because it holds data
     // owned by the HphpArray
     propVal->m_data.num = tvResult.m_data.num;
@@ -1449,7 +1449,7 @@ void ObjectData::cloneSet(ObjectData* clone) {
       TypedValue* val = props->nvGet(strKey);
       TypedValue* retval;
       auto cloneProps = clone->o_properties.get();
-      cloneProps->createLvalPtr(strKey, *(Variant**)&retval, false);
+      cloneProps->lval(strKey, *(Variant**)&retval, false);
       tvDupFlattenVars(val, retval, cloneProps);
       iter = o_properties.get()->iter_advance(iter);
       decRefStr(strKey);

hphp/runtime/base/policy_array.cpp

@@ -333,25 +333,6 @@ ArrayData *PolicyArray::LvalNew(ArrayData* ad, Variant *&ret, bool copy) {
   return a;
 }
 
-ArrayData *PolicyArray::CreateLvalPtr(ArrayData* ad, StringData* k,
-                                      Variant *&ret, bool copy) {
-  auto a = asPolicyArray(ad);
-  APILOG(a) << "(" << keystr(k) << ", " << ret << ", " << copy << ")";
-  return a->addLval(k, ret, copy);
-}
-
-ArrayData *PolicyArray::GetLvalPtr(ArrayData* ad, StringData* k,
-                                   Variant *&ret, bool copy) {
-  auto a = asPolicyArray(ad);
-  APILOG(a) << "(" << keystr(k) << ", " << ret << ", " << copy << ")";
-  if (copy) a = asPolicyArray(Copy(a));
-  const auto pos = a->find(k, a->m_size);
-  ret = pos != PosType::invalid
-    ? &a->Store::lval(pos)
-    : nullptr;
-  return a;
-}
-
 template <class K>
 PolicyArray* PolicyArray::setImpl(K k, const Variant& v, bool copy) {
   APILOG(this) << "(" << keystr(k) << ", " << valstr(v) << ", " << copy

hphp/runtime/base/policy_array.h

@@ -385,16 +385,6 @@ public:
    */
   static ArrayData *LvalNew(ArrayData* ad, Variant *&ret, bool copy);
 
-  /**
-   * Helper functions used for getting a reference to elements of
-   * the dynamic property array in ObjectData or the local cache array
-   * in ShardMap.
-   */
-  static ArrayData *CreateLvalPtr(ArrayData*, StringData* k, Variant *&ret,
-                                  bool copy);
-  static ArrayData *GetLvalPtr(ArrayData*, StringData* k, Variant *&ret,
-                               bool copy);
-
   /**
    * Setting a value at specified key. If "copy" is true, make a copy first
    * then set the value. Return NULL if escalation is not needed, or an

hphp/runtime/base/type_array.cpp

@@ -487,21 +487,6 @@ Variant Array::rvalAt(CVarRef key, ACCESSPARAMS_IMPL) const {
   return Array::rvalAtRef(key, flags);
 }
 
-Variant *Array::createLvalPtr(CStrRef key, bool forWrite) {
-  if (!m_px) ArrayBase::operator=(ArrayData::Create());
-  return &lvalAt(key, AccessFlags::Key);
-}
-
-Variant *Array::getLvalPtr(CStrRef key, bool forWrite) {
-  Variant *ret = nullptr;
-  if (m_px) {
-    ArrayData *escalated = m_px->getLvalPtr(key, ret,
-                                            forWrite && m_px->getCount() > 1);
-    if (escalated != m_px) ArrayBase::operator=(escalated);
-  }
-  return ret;
-}
-
 Variant &Array::lvalAt() {
   if (!m_px) ArrayBase::operator=(ArrayData::Create());
   Variant *ret = nullptr;

hphp/runtime/base/type_array.h

@@ -298,9 +298,6 @@ class Array : protected SmartPtr<ArrayData> {
     return *ret;
   }
 
-  Variant *createLvalPtr(CStrRef key, bool forWrite);
-  Variant *getLvalPtr(CStrRef key, bool forWrite);
-
   Variant &lvalAt();
 
   Variant &lvalAt(int     key, ACCESSPARAMS_DECL) {

hphp/runtime/base/type_variant.cpp

@@ -1219,22 +1219,6 @@ Variant &Variant::lvalRef(CVarRef k, Variant& tmp, ACCESSPARAMS_IMPL) {
   return Variant::LvalAtImpl0<CVarRef>(this, k, &tmp, false, flags);
 }
 
-Variant *Variant::createLvalPtr(CStrRef key, bool forWrite) {
-  Variant *t = m_type == KindOfRef ? m_data.pref->var() : this;
-  if (t->m_type == KindOfArray) {
-    return t->asArrRef().createLvalPtr(key, forWrite);
-  }
-  return nullptr;
-}
-
-Variant *Variant::getLvalPtr(CStrRef key, bool forWrite) {
-  Variant *t = m_type == KindOfRef ? m_data.pref->var() : this;
-  if (t->m_type == KindOfArray) {
-    return t->asArrRef().getLvalPtr(key, forWrite);
-  }
-  return nullptr;
-}
-
 Variant &Variant::lvalAt() {
   switch (m_type) {
   case KindOfUninit:

hphp/runtime/base/type_variant.h

@@ -789,9 +789,6 @@ class Variant : private TypedValue {
     return *ret;
   }
 
-  Variant *createLvalPtr(CStrRef key, bool forWrite);
-  Variant *getLvalPtr(CStrRef key, bool forWrite);
-
   Variant &lvalAt();
 
   static Variant &lvalInvalid();

hphp/runtime/ext/ext_fb.cpp

@@ -1651,9 +1651,13 @@ static Array const_data;
 
 Variant f_fb_const_fetch(CVarRef key) {
   String k = key.toString();
-  Variant *ret = const_data.getLvalPtr(k, false);
-  if (ret) return *ret;
-  return false;
+  if (ArrayData* ad = const_data.get()) {
+    auto& v = ad->get(k, /*error*/false);
+    if (&v != &null_variant) {
+      return v;
+    }
+  }
+  return Variant(false);
 }
 
 void const_load_set(CStrRef key, CVarRef value) {