Implement inc/dec outside of variant

hphp/runtime/base/tv_arith.cpp

@@ -287,6 +287,108 @@ void cellBitOpEq(Op op, Cell& c1, Cell c2) {
   cellSet(result, c1);
 }
 
+// Op must implement the interface described for cellIncDecOp.
+template<class Op>
+void stringIncDecOp(Op op, Cell& cell) {
+  assert(IS_STRING_TYPE(cell.m_type));
+
+  auto const sd = cell.m_data.pstr;
+  if (sd->empty()) {
+    decRefStr(sd);
+    cellCopy(op.emptyString(), cell);
+    return;
+  }
+
+  int64_t ival;
+  double dval;
+  auto const dt = sd->isNumericWithVal(ival, dval, true /* allow_errors */);
+  switch (dt) {
+  case KindOfInt64:
+    decRefStr(sd);
+    op(ival);
+    cellCopy(num(ival), cell);
+    break;
+  case KindOfDouble:
+    decRefStr(sd);
+    op(dval);
+    cellCopy(dbl(dval), cell);
+    break;
+  default:
+    assert(dt == KindOfNull);
+    op.nonNumericString(cell);
+    break;
+  }
+}
+
+/*
+ * Inc or Dec for a string, depending on Op.  Op must implement
+ *
+ *   - a function call operator for numeric types
+ *   - a nullCase(Cell&) function that returns the result for null types
+ *   - an emptyString() function that performs the operation for empty strings
+ *   - and a nonNumericString(Cell&) function used for non-numeric strings
+ *
+ * PHP's Inc and Dec behave differently in all these cases, so this
+ * abstracts out the common parts from those differences.
+ */
+template<class Op>
+void cellIncDecOp(Op op, Cell& cell) {
+  assert(cellIsPlausible(&cell));
+
+  switch (cell.m_type) {
+  case KindOfInt64:
+    op(cell.m_data.num);
+    break;
+  case KindOfDouble:
+    op(cell.m_data.dbl);
+    break;
+
+  case KindOfString:
+  case KindOfStaticString:
+    stringIncDecOp(op, cell);
+    break;
+
+  case KindOfUninit:
+  case KindOfNull:
+    op.nullCase(cell);
+    break;
+
+  case KindOfBoolean:
+  case KindOfObject:
+  case KindOfArray:
+    break;
+  default:
+    not_reached();
+  }
+}
+
+const StaticString s_1("1");
+
+struct Inc {
+  template<class T> void operator()(T& t) const { ++t; }
+  void nullCase(Cell& cell) const { cellCopy(num(1), cell); }
+
+  Cell emptyString() const {
+    return make_tv<KindOfStaticString>(s_1.get());
+  }
+
+  void nonNumericString(Cell& cell) const {
+    auto const sd = cell.m_data.pstr;
+    auto const newSd = NEW(StringData)(sd, CopyString);
+    newSd->inc();
+    newSd->incRefCount();
+    decRefStr(sd);
+    cellCopy(make_tv<KindOfString>(newSd), cell);
+  }
+};
+
+struct Dec {
+  template<class T> void operator()(T& t) const { --t; }
+  void nullCase(Cell&) const {}
+  Cell emptyString() const { return num(-1); }
+  void nonNumericString(Cell&) const {}
+};
+
 }
 
 //////////////////////////////////////////////////////////////////////
@@ -379,6 +481,14 @@ void cellBitXorEq(Cell& c1, Cell c2) {
   cellBitOpEq(cellBitXor, c1, c2);
 }
 
+void cellInc(Cell& cell) {
+  cellIncDecOp(Inc(), cell);
+}
+
+void cellDec(Cell& cell) {
+  cellIncDecOp(Dec(), cell);
+}
+
 //////////////////////////////////////////////////////////////////////
 
 }

hphp/runtime/base/tv_arith.h

@@ -112,6 +112,15 @@ void cellBitAndEq(Cell& c1, Cell);
 void cellBitOrEq(Cell& c1, Cell);
 void cellBitXorEq(Cell& c1, Cell);
 
+/*
+ * PHP operator ++ and --.
+ *
+ * Mutates the argument in place, with the effects of php's
+ * pre-increment or pre-decrement operators.
+ */
+void cellInc(Cell&);
+void cellDec(Cell&);
+
 //////////////////////////////////////////////////////////////////////
 
 }

hphp/runtime/base/type_variant.cpp

@@ -541,85 +541,6 @@ Variant Variant::bitNot() const {
   throw InvalidOperandException("only numerics and strings can be negated");
 }
 
-///////////////////////////////////////////////////////////////////////////////
-// increment/decrement
-
-Variant &Variant::operator++() {
-  switch (getType()) {
-  case KindOfUninit:
-  case KindOfNull:   set(int64_t(1)); break;
-  case KindOfInt64:  set(toInt64() + 1);  break;
-  case KindOfDouble: set(toDouble() + 1); break;
-  case KindOfStaticString:
-  case KindOfString:
-    {
-      if (getStringData()->empty()) {
-        set(s_1);
-      } else {
-        int64_t lval; double dval;
-        DataType ret = convertToNumeric(&lval, &dval);
-        switch (ret) {
-        case KindOfInt64:  set(lval + 1); break;
-        case KindOfDouble: set(dval + 1); break;
-        case KindOfUninit:
-        case KindOfNull:
-          split();
-          getStringData()->inc(); break;
-        default:
-          assert(false);
-          break;
-        }
-      }
-    }
-    break;
-  default:
-    break;
-  }
-  return *this;
-}
-
-Variant Variant::operator++(int) {
-  Variant ret(*this);
-  operator++();
-  return ret;
-}
-
-Variant &Variant::operator--() {
-  switch (getType()) {
-  case KindOfInt64:  set(toInt64() - 1);  break;
-  case KindOfDouble: set(toDouble() - 1); break;
-  case KindOfStaticString:
-  case KindOfString:
-    {
-      if (getStringData()->empty()) {
-        set(int64_t(-1LL));
-      } else {
-        int64_t lval; double dval;
-        DataType ret = convertToNumeric(&lval, &dval);
-        switch (ret) {
-        case KindOfInt64:  set(lval - 1);   break;
-        case KindOfDouble: set(dval - 1);   break;
-        case KindOfUninit:
-        case KindOfNull:   /* do nothing */ break;
-        default:
-          assert(false);
-          break;
-        }
-      }
-    }
-    break;
-  default:
-    break;
-  }
-  return *this;
-}
-
-Variant Variant::operator--(int) {
-  Variant ret(*this);
-  operator--();
-  return ret;
-}
-
 ///////////////////////////////////////////////////////////////////////////////
 // iterator functions
 

hphp/runtime/base/type_variant.h

@@ -585,10 +585,10 @@ class Variant : private TypedValue {
   Variant &operator <<=(int64_t n) = delete;
   Variant &operator >>=(int64_t n) = delete;
 
-  Variant &operator ++ ();
-  Variant  operator ++ (int);
-  Variant &operator -- ();
-  Variant  operator -- (int);
+  Variant &operator ++ () = delete;
+  Variant  operator ++ (int) = delete;
+  Variant &operator -- () = delete;
+  Variant  operator -- (int) = delete;
 
   // Return the result of applying the php bitwise not operator to
   // this value.

hphp/runtime/vm/member_operations.h

@@ -997,45 +997,11 @@ inline TypedValue* SetOpNewElem(TypedValue& tvScratch, TypedValue& tvRef,
 }
 
 template <bool setResult>
-inline void IncDecBody(unsigned char op, TypedValue* fr,
-                       TypedValue* to) {
-  if (fr->m_type == KindOfInt64) {
-    switch ((IncDecOp)op) {
-    case PreInc: {
-      ++(fr->m_data.num);
-      if (setResult) {
-        cellDup(*fr, *to);
-      }
-      break;
-    }
-    case PostInc: {
-      if (setResult) {
-        cellDup(*fr, *to);
-      }
-      ++(fr->m_data.num);
-      break;
-    }
-    case PreDec: {
-      --(fr->m_data.num);
-      if (setResult) {
-        cellDup(*fr, *to);
-      }
-      break;
-    }
-    case PostDec: {
-      if (setResult) {
-        cellDup(*fr, *to);
-      }
-      --(fr->m_data.num);
-      break;
-    }
-    default: assert(false);
-    }
-    return;
-  }
+NEVER_INLINE
+void incDecBodySlow(unsigned char op, TypedValue* fr, TypedValue* to) {
   if (fr->m_type == KindOfUninit) {
     ActRec* fp = g_vmContext->m_fp;
-    size_t pind = ((uintptr_t(fp) - uintptr_t(fr)) / sizeof(TypedValue)) - 1;
+    size_t pind = reinterpret_cast<TypedValue*>(fp) - fr - 1;
     if (pind < size_t(fp->m_func->numNamedLocals())) {
       // Only raise a warning if fr points to a local variable
       raise_notice(Strings::UNDEFINED_VARIABLE,
@@ -1044,38 +1010,77 @@ inline void IncDecBody(unsigned char op, TypedValue* fr,
     // Convert uninit null to null so that we don't write out an uninit null
     // to the eval stack for PostInc and PostDec.
     fr->m_type = KindOfNull;
+  } else {
+    fr = tvToCell(fr);
   }
-  switch ((IncDecOp)op) {
-  case PreInc: {
-    ++(tvAsVariant(fr));
+
+  assert(cellIsPlausible(fr));
+
+  switch (static_cast<IncDecOp>(op)) {
+  case PreInc:
+    cellInc(*fr);
     if (setResult) {
-      tvReadCell(fr, to);
+      cellDup(*fr, *to);
     }
     break;
-  }
-  case PostInc: {
+  case PostInc:
     if (setResult) {
-      tvReadCell(fr, to);
+      cellDup(*fr, *to);
     }
-    ++(tvAsVariant(fr));
+    cellInc(*fr);
     break;
-  }
-  case PreDec: {
-    --(tvAsVariant(fr));
+  case PreDec:
+    cellDec(*fr);
     if (setResult) {
-      tvReadCell(fr, to);
+      cellDup(*fr, *to);
     }
     break;
-  }
-  case PostDec: {
+  case PostDec:
     if (setResult) {
-      tvReadCell(fr, to);
+      cellDup(*fr, *to);
     }
-    --(tvAsVariant(fr));
+    cellDec(*fr);
+    break;
+  case IncDec_invalid:
+    not_reached();
+  }
+}
+
+template <bool setResult>
+inline void IncDecBody(unsigned char op, TypedValue* fr, TypedValue* to) {
+  if (UNLIKELY(fr->m_type != KindOfInt64)) {
+    return incDecBodySlow<setResult>(op, fr, to);
+  }
+
+  switch (static_cast<IncDecOp>(op)) {
+  case PreInc:
+    ++fr->m_data.num;
+    if (setResult) {
+      cellCopy(*fr, *to);
+    }
+    return;
+  case PostInc:
+    if (setResult) {
+      cellCopy(*fr, *to);
+    }
+    ++fr->m_data.num;
+    return;
+  case PreDec:
+    --fr->m_data.num;
+    if (setResult) {
+      cellCopy(*fr, *to);
+    }
+    return;
+  case PostDec:
+    if (setResult) {
+      cellCopy(*fr, *to);
+    }
+    --fr->m_data.num;
+    return;
+  case IncDec_invalid:
     break;
   }
-  default: assert(false);
-  }
+  not_reached();
 }
 
 template <bool setResult>

hphp/test/ext/test_cpp_base.cpp

@@ -344,12 +344,6 @@ bool TestCppBase::TestArray() {
     arr.lvalAt(name) = String("value");
     VS(arr, CREATE_MAP1("name", "value"));
   }
-  {
-    Array arr;
-    arr.lvalAt(s_A) = 10;
-    arr.lvalAt(s_A)++;
-    VS(arr[s_A], 11);
-  }
 
   {
     Array arr;
@@ -741,21 +735,6 @@ bool TestCppBase::TestVariant() {
     v1 = 20;
     VS(v2[1], 20);
   }
-  {
-    Variant v1 = 10;
-    Variant v2 = strongBind(v1);
-    v2++;
-    VS(v2, 11);
-    VS(v1, 11);
-  }
-  {
-    Variant arr = CREATE_VECTOR2(1, 2);
-    Variant v;
-    for (MutableArrayIter iter = arr.begin(nullptr, v); iter.advance();) {
-      v++;
-    }
-    VS(arr, CREATE_VECTOR2(2, 3));
-  }
 
   // array escalation
   {