Fix move constructor Value and fix the hash iterator

4 files changed, 33 insertions, 58 deletions

diff --git a/include/valueiterator.h b/include/valueiterator.h
index 9e51f15..12fd67a 100644
--- a/include/valueiterator.h
+++ b/include/valueiterator.h
@@ -31,7 +31,7 @@ public:
      *  Constructor
      *  @param  impl        Implementation iterator
      */
-    ValueIterator(ValueIteratorImpl *impl) : _impl(impl) {}
+    ValueIterator(ValueIteratorImpl *impl);
 
     /**
      *  Copy constructor
@@ -117,9 +117,9 @@ public:
 private:
     /**
      *  Pointer to the actual implementation
-     *  @var    std::unique_ptr
+     *  @var    std::unique_ptr<ValueIteratorImpl>
      */
-    ValueIteratorImpl *_impl;
+    std::unique_ptr<ValueIteratorImpl> _impl;
 
 };
 
diff --git a/zend/hashiterator.h b/zend/hashiterator.h
index 357ffa6..ccc7d30 100644
--- a/zend/hashiterator.h
+++ b/zend/hashiterator.h
@@ -223,6 +223,10 @@ private:
      */
     bool invalidate()
     {
+        // set position to be one after the end
+        zend_hash_internal_pointer_end_ex(_table, &_position);
+        zend_hash_move_forward_ex(_table, &_position);
+
         // no longer valid
         _valid = false;
 
diff --git a/zend/value.cpp b/zend/value.cpp
index d87e576..00fe969 100644
--- a/zend/value.cpp
+++ b/zend/value.cpp
@@ -315,59 +315,28 @@ Value &Value::operator=(Value &&value) _NOEXCEPT
     // skip self assignment
     if (this == &value) return *this;
 
-    // is the object a reference?
-    if (_val && Z_ISREF_P(_val))
+    // if neither value is a reference we can simply swap the values
+    // the other value will then destruct and reduce the refcount
+    if (!Z_ISREF_P(value._val) && (!_val || !Z_ISREF_P(_val)))
     {
-        // @todo difference if the other object is a reference or not?
-
-        // the current object is a reference, this means that we should
-        // keep the zval object, and copy the other value into it, get
-        // the current refcount
-        int refcount = Z_REFCOUNT_P(_val);
-
-        // make the copy
-        *_val = *value._val;
-
-        // restore reference and refcount setting
-        ZVAL_MAKE_REF(_val);
-        Z_SET_REFCOUNT_P(_val, refcount);
-
-        // how many references did the old variable have?
-        if (Z_REFCOUNT_P(value._val) > 1)
-        {
-            // the other object already had multiple references, this
-            // implies that many other PHP variables are also referring
-            // to it, and we still need to store its contents, with one
-            // reference less
-            Z_DELREF_P(value._val);
-
-            // and we need to run the copy constructor on the current
-            // value, because we're making a deep copy
-            zval_copy_ctor(_val);
-        }
-        else
-        {
-            // the last and only reference to the other object was
-            // removed, we no longer need it
-            delete value._val;
-
-            // the other object is no longer valid
-            value._val = nullptr;
-        }
+        // just swap the pointer
+        std::swap(_val, value._val);
+    }
+    else if (_val)
+    {
+        // copy the value over to our local zval
+        ZVAL_COPY_VALUE(_val, value._val);
     }
     else
     {
-        // first destruct ourselves properly
-        this->~Value();
+        // first swap the value out
+        std::swap(_val, value._val);
 
-        // just copy the zval completely
-        _val = value._val;
-
-        // the other object is no longer valid
-        value._val = nullptr;
+        // and make sure it is no longer a reference
+        ZVAL_UNREF(_val);
     }
 
-    // done
+    // allow chaining
     return *this;
 }
 
diff --git a/zend/valueiterator.cpp b/zend/valueiterator.cpp
index 65c687c..e020f21 100644
--- a/zend/valueiterator.cpp
+++ b/zend/valueiterator.cpp
@@ -14,6 +14,12 @@
 namespace Php {
 
 /**
+ *  Constructor
+ *  @param  impl        Implementation iterator
+ */
+ValueIterator::ValueIterator(ValueIteratorImpl *impl) : _impl(impl) {}
+
+/**
  *  Copy constructor
  *  @param  that
  *  @param  tsrm_ls
@@ -23,11 +29,7 @@ ValueIterator::ValueIterator(const ValueIterator &that) : _impl(that._impl->clon
 /**
  *  Destructor
  */
-ValueIterator::~ValueIterator()
-{
-    // get rid of implementation
-    delete _impl;
-}
+ValueIterator::~ValueIterator() = default;
 
 /**
  *  Increment position
@@ -37,7 +39,7 @@ ValueIterator &ValueIterator::operator++()
 {
     // increment implementation
     _impl->increment();
-    
+
     // done
     return *this;
 }
@@ -50,7 +52,7 @@ ValueIterator &ValueIterator::operator--()
 {
     // decrement implementation
     _impl->decrement();
-    
+
     // done
     return *this;
 }
@@ -62,7 +64,7 @@ ValueIterator &ValueIterator::operator--()
  */
 bool ValueIterator::operator==(const ValueIterator &that) const
 {
-    return _impl->equals(that._impl);
+    return _impl->equals(that._impl.get());
 }
 
 /**
@@ -72,7 +74,7 @@ bool ValueIterator::operator==(const ValueIterator &that) const
  */
 bool ValueIterator::operator!=(const ValueIterator &that) const
 {
-    return !_impl->equals(that._impl);
+    return !_impl->equals(that._impl.get());
 }
 
 /**