moved implementation for hashiterator to header file, introduced invaliditerator class, valueiterator now uses the hashiterator class internally

8 files changed, 226 insertions, 352 deletions

diff --git a/include/valueiterator.h b/include/valueiterator.h
index 62ec175..00ecc39 100644
--- a/include/valueiterator.h
+++ b/include/valueiterator.h
@@ -23,6 +23,11 @@ struct bucket;
 namespace Php {
 
 /**
+ *  Forward declarations
+ */
+class IteratorImpl;
+
+/**
  *  Class definition
  */
 class ValueIterator
@@ -30,10 +35,9 @@ class ValueIterator
 public:
     /**
      *  Constructor
-     *  @param  hashtable       The hashtable to iterate over
-     *  @param  first           Should it start on the first position?
+     *  @param  impl        Implementation iterator
      */
-    ValueIterator(struct _hashtable *hashtable, bool first);
+    ValueIterator(IteratorImpl *impl) : _impl(impl) {}
     
     /**
      *  Copy constructor
@@ -95,69 +99,34 @@ public:
      *  @param  that
      *  @return bool
      */
-    bool operator==(const ValueIterator &that) const
-    {
-        return _position == that._position;
-    }
+    bool operator==(const ValueIterator &that) const;
 
     /**
      *  Compare with other iterator
      *  @param  that
      *  @return bool
      */
-    bool operator!=(const ValueIterator &that) const
-    {
-        return _position != that._position;
-    }
+    bool operator!=(const ValueIterator &that) const;
 
     /**
      *  Derefecence, this returns a std::pair with the current key and value
      *  @return std::pair
      */
-    const std::pair<Value,Value> &operator*() const
-    {
-        return _current;
-    }
+    const std::pair<Value,Value> &operator*() const;
     
     /**
      *  Dereference, this returns a std::pair with the current key and value
      *  @return std::pair
      */
-    const std::pair<Value,Value> *operator->() const
-    {
-        return &_current;
-    }
+    const std::pair<Value,Value> *operator->() const;
 
 private:
     /**
-     *  The hash table over which is being iterated
-     *  @var    HashTable
-     */
-    struct _hashtable *_table = nullptr;
-
-    /**
-     *  The position in the hash table
-     *  @var    HashPosition
-     */
-    struct bucket *_position = nullptr;
-    
-    /**
-     *  The current key and value
-     *  @var    std::pair
-     */
-    std::pair<Value,Value> _current;
-
-    /**
-     *  Read current key and value
-     *  @return bool
+     *  Pointer to the actual implementation
+     *  @var    std::unique_ptr
      */
-    bool read();
+    std::unique_ptr<IteratorImpl> _impl;
 
-    /**
-     *  Invalidate the iterator
-     *  @return bool
-     */
-    bool invalidate();
 };
 
 /**
diff --git a/src/hashiterator.cpp b/src/hashiterator.cpp
deleted file mode 100644
index 43a9d27..0000000
--- a/src/hashiterator.cpp
+++ /dev/null
@@ -1,168 +0,0 @@
-/**
- *  HashIterator.cpp
- *
- *  Implementation of the value iterator
- *
- *  @author Emiel Bruijntjes <emiel.bruijntjes@copernica.com>
- *  @copyright 2014 Copernica BV
- */
-#include "includes.h"
-
-/**
- *  Set up namespace
- */
-namespace Php {
-
-/**
- *  Constructor
- *  @param  hashtable       The hashtable to iterate over
- *  @param  first           Should it start at the first position?
- */
-HashIterator::HashIterator(HashTable *hashtable, bool first) : _table(hashtable)
-{
-    // reset the hash pointer to the internal position
-    if (hashtable && first) 
-    {
-        // move to first position
-        zend_hash_internal_pointer_reset_ex(_table, &_position);
-        
-        // read current data
-        if (read()) return;
-        
-        // data was private, move on
-        increment();
-    }
-    else
-    {
-        // start with invalid data
-        invalidate();
-    }
-}
-
-/**
- *  Increment position
- *  @return bool
- */
-bool HashIterator::increment()
-{
-    // leap out if already on an invalid pos (behind the last pos)
-    if (!_position) return false;
-    
-    // move the iterator forward
-    if (zend_hash_move_forward_ex(_table, &_position) == SUCCESS)
-    {
-        // read current key and value
-        if (read()) return true;
-        
-        // data was private or invalid, move further
-        return increment();
-    }
-    else
-    {
-        // invalidate current position
-        return invalidate();
-    }
-}
-
-/**
- *  Decrement position
- *  @return bool
- */
-bool HashIterator::decrement()
-{
-    // leap out if we're not even iterating over a hash table
-    if (!_table) return false;
-    
-    // if position is invalid, it is one position behind the last position
-    if (!_position)
-    {
-        // move to last position
-        zend_hash_internal_pointer_end_ex(_table, &_position);
-    }
-    else if (zend_hash_move_backwards_ex(_table, &_position) == FAILURE)
-    {
-        // invalidate current position
-        return invalidate();
-    }
-
-    // read current key and value
-    if (read()) return true;
-    
-    // data was private, move on
-    return decrement();
-}
-
-/**
- *  Read current key and value
- *  @return bool        true if the object is in a valid position, false otherwise
- */
-bool HashIterator::read()
-{
-    // zval to read the current key in
-    Value key;
-
-#if PHP_VERSION_ID >= 50500
-
-    // read in the current key
-    zend_hash_get_current_key_zval_ex(_table, key._val, &_position);
-    
-    // if the key is set to NULL, it means that the object is not at a valid position
-    if (key.isNull()) return invalidate();
-    
-#else
-
-    // php 5.3 and php 5.4 need a different implementation because the function
-    // zend_hash_get_current_key_zval_ex is missing in php 5.3, declare variables
-    // we need for storing the key in
-    char *string_key;
-    unsigned int str_len;
-    unsigned long num_key;
-    
-    // get the current key
-    int type = zend_hash_get_current_key_ex(_table, &string_key, &str_len, &num_key, 0, &_position);
-    
-    // if key is not found, the iterator is at an invalid position
-    if (type == HASH_KEY_NON_EXISTANT) return invalidate();
-
-    // numeric keys are the easiest ones
-    if (type == HASH_KEY_IS_LONG) key = (int64_t)num_key;
-    else key = string_key;
-
-#endif
-
-    // iterator is at a valid position, go fetch the data
-    // this is the variable we need for fetching the data
-    zval **value;
-            
-    // retrieve data
-    zend_hash_get_current_data_ex(_table, (void **) &value, &_position);
-    
-    // we can now update the current data
-    _current = std::make_pair<Value,Value>(std::move(key), *value);
-
-    // if the key is private (it starts with a null character) we should return
-    // false to report that the object is not in a completely valid state
-    return !_current.first.isString() || _current.first.rawValue()[0];
-}
-
-/**
- *  Invalidate the iterator
- *  @return bool        always false
- */
-bool HashIterator::invalidate()
-{
-    // forget current position
-    _position = nullptr;
-    
-    // make the data a pair of null ptrs
-    _current = std::make_pair<Value,Value>(nullptr,nullptr);
-    
-    // done
-    return false;
-}
-
-/**
- *  End namespace
- */
-}
-
diff --git a/src/hashiterator.h b/src/hashiterator.h
index c37c22b..3c23003 100644
--- a/src/hashiterator.h
+++ b/src/hashiterator.h
@@ -13,12 +13,6 @@
  */
 
 /**
- *  Forward declaration
- */
-struct _hashtable;
-struct bucket;
-
-/**
  *  Set up namespace
  */
 namespace Php {
@@ -34,30 +28,105 @@ public:
      *  @param  hashtable       The hashtable to iterate over
      *  @param  first           Should it start on the first position?
      */
-    HashIterator(struct _hashtable *hashtable, bool first);
+    HashIterator(HashTable *hashtable, bool first) : _table(hashtable)
+    {
+        // reset the hash pointer to the internal position
+        if (hashtable && first) 
+        {
+            // move to first position
+            zend_hash_internal_pointer_reset_ex(_table, &_position);
+            
+            // read current data
+            if (read()) return;
+            
+            // data was private, move on
+            increment();
+        }
+        else
+        {
+            // start with invalid data
+            invalidate();
+        }
+    }
     
     /**
      *  Copy constructor
      *  @param  that
      */
-    HashIterator(const ValueIterator &that);
+    HashIterator(const HashIterator &that) :
+        _table(that._table), _position(that._position)
+    {
+        // read current position
+        read();
+    }
     
     /**
      *  Destructor
      */
     virtual ~HashIterator() {}
+    
+    /**
+     *  Clone the object
+     *  @return IteratorImpl
+     */
+    virtual IteratorImpl *clone()
+    {
+        // create a new instance
+        return new HashIterator(*this);
+    }
 
     /**
      *  Increment position (pre-increment)
      *  @return bool
      */
-    virtual bool increment() override;
+    virtual bool increment() override
+    {
+        // leap out if already on an invalid pos (behind the last pos)
+        if (!_position) return false;
+        
+        // move the iterator forward
+        if (zend_hash_move_forward_ex(_table, &_position) == SUCCESS)
+        {
+            // read current key and value
+            if (read()) return true;
+            
+            // data was private or invalid, move further
+            return increment();
+        }
+        else
+        {
+            // invalidate current position
+            return invalidate();
+        }
+    }
     
     /**
      *  Decrement position (pre-decrement)
      *  @return bool
      */
-    virtual bool decrement() override;
+    virtual bool decrement() override
+    {
+        // leap out if we're not even iterating over a hash table
+        if (!_table) return false;
+        
+        // if position is invalid, it is one position behind the last position
+        if (!_position)
+        {
+            // move to last position
+            zend_hash_internal_pointer_end_ex(_table, &_position);
+        }
+        else if (zend_hash_move_backwards_ex(_table, &_position) == FAILURE)
+        {
+            // invalidate current position
+            return invalidate();
+        }
+
+        // read current key and value
+        if (read()) return true;
+        
+        // data was private, move on
+        return decrement();
+    }
 
     /**
      *  Compare with other iterator
@@ -87,13 +156,13 @@ private:
      *  The hash table over which is being iterated
      *  @var    HashTable
      */
-    struct _hashtable *_table = nullptr;
+    HashTable *_table = nullptr;
 
     /**
      *  The position in the hash table
      *  @var    HashPosition
      */
-    struct bucket *_position = nullptr;
+    Bucket *_position = nullptr;
     
     /**
      *  The current key and value
diff --git a/src/includes.h b/src/includes.h
index e59d67b..1862037 100644
--- a/src/includes.h
+++ b/src/includes.h
@@ -94,6 +94,7 @@
 #include "property.h"
 #include "iteratorimpl.h"
 #include "hashiterator.h"
+#include "invaliditerator.h"
 
 #ifndef ZVAL_COPY_VALUE
 #define ZVAL_COPY_VALUE(z, v)  \
diff --git a/src/invaliditerator.h b/src/invaliditerator.h
new file mode 100644
index 0000000..61815ff
--- /dev/null
+++ b/src/invaliditerator.h
@@ -0,0 +1,80 @@
+/**
+ *  InvalidIterator.h
+ *
+ *  Iterator class that is used for value objects that are not even
+ *  iteratable.
+ *
+ *  @author Emiel Bruijntjes <emiel.bruijntjes@copernica.com>
+ *  @copyright 2014 Copernica BV
+ */
+
+/**
+ *  Set up namespace
+ */
+namespace Php {
+
+/**
+ *  Class definition
+ */
+class InvalidIterator : public IteratorImpl
+{
+public:
+    /**
+     *  Clone the object
+     *  @return IteratorImpl
+     */
+    virtual IteratorImpl *clone()
+    {
+        // create a new instance
+        return new InvalidIterator(*this);
+    }
+
+    /**
+     *  Increment position (pre-increment)
+     *  @return bool
+     */
+    virtual bool increment() override
+    {
+        return false;
+    }
+    
+    /**
+     *  Decrement position (pre-decrement)
+     *  @return bool
+     */
+    virtual bool decrement() override
+    {
+        return false;
+    }
+
+    /**
+     *  Compare with other iterator
+     *  @param  that
+     *  @return bool
+     */
+    virtual bool equals(const IteratorImpl *that) const override
+    {
+        // the other iterator is also an invalid-iterator, and all invalid
+        // iterators are equal
+        return true;
+    }
+
+    /**
+     *  Derefecence, this returns a std::pair with the current key and value
+     *  @return std::pair
+     */
+    virtual const std::pair<Value,Value> &current() const override
+    {
+        // this method is never called, when it is, we create a static instance
+        static std::pair<Value,Value> result;
+    
+        // return it
+        return result;
+    }
+};
+
+/**
+ *  End namespace
+ */
+}
+
diff --git a/src/iteratorimpl.h b/src/iteratorimpl.h
index 97c59e4..4d15c73 100644
--- a/src/iteratorimpl.h
+++ b/src/iteratorimpl.h
@@ -19,6 +19,13 @@ namespace Php {
  */
 class IteratorImpl
 {
+public:
+    /**
+     *  Clone the object
+     *  @return IteratorImpl*
+     */
+    virtual IteratorImpl *clone() = 0;
+
     /**
      *  Increment position (pre-increment)
      *  @return bool
diff --git a/src/value.cpp b/src/value.cpp
index 356df4a..96f7321 100644
--- a/src/value.cpp
+++ b/src/value.cpp
@@ -1550,13 +1550,13 @@ std::map<std::string,Php::Value> Value::mapValue() const
 ValueIterator Value::begin() const
 {
     // check type
-    if (isArray()) return ValueIterator(Z_ARRVAL_P(_val), true);
+    if (isArray()) return ValueIterator(new HashIterator(Z_ARRVAL_P(_val), true));
     
     // get access to the hast table
-    if (isObject()) return ValueIterator(Z_OBJ_HT_P(_val)->get_properties(_val), true);
+    if (isObject()) return ValueIterator(new HashIterator(Z_OBJ_HT_P(_val)->get_properties(_val), true));
     
     // invalid
-    return ValueIterator(nullptr,true);
+    return ValueIterator(new InvalidIterator());
 }
 
 /**
@@ -1567,13 +1567,13 @@ ValueIterator Value::begin() const
 ValueIterator Value::end() const
 {
     // check type
-    if (isArray()) return ValueIterator(Z_ARRVAL_P(_val), false);
+    if (isArray()) return ValueIterator(new HashIterator(Z_ARRVAL_P(_val), false));
     
     // get access to the hast table
-    if (isObject()) return ValueIterator(Z_OBJ_HT_P(_val)->get_properties(_val), false);
+    if (isObject()) return ValueIterator(new HashIterator(Z_OBJ_HT_P(_val)->get_properties(_val), false));
     
     // invalid
-    return ValueIterator(nullptr, false);
+    return ValueIterator(new InvalidIterator());
 }
 
 /**
diff --git a/src/valueiterator.cpp b/src/valueiterator.cpp
index 4fe1ee8..92a2542 100644
--- a/src/valueiterator.cpp
+++ b/src/valueiterator.cpp
@@ -14,30 +14,10 @@
 namespace Php {
 
 /**
- *  Constructor
- *  @param  hashtable       The hashtable to iterate over
- *  @param  first           Should it start at the first position?
+ *  Copy constructor
+ *  @param  that
  */
-ValueIterator::ValueIterator(HashTable *hashtable, bool first) : _table(hashtable)
-{
-    // reset the hash pointer to the internal position
-    if (hashtable && first) 
-    {
-        // move to first position
-        zend_hash_internal_pointer_reset_ex(_table, &_position);
-        
-        // read current data
-        if (read()) return;
-        
-        // data was private, move on
-        operator++();
-    }
-    else
-    {
-        // start with invalid data
-        invalidate();
-    }
-}
+ValueIterator::ValueIterator(const ValueIterator &that) : _impl(that._impl->clone()) {}
 
 /**
  *  Increment position
@@ -45,26 +25,11 @@ ValueIterator::ValueIterator(HashTable *hashtable, bool first) : _table(hashtabl
  */
 ValueIterator &ValueIterator::operator++()
 {
-    // leap out if already on an invalid pos (behind the last pos)
-    if (!_position) return *this;
+    // increment implementation
+    _impl->increment();
     
-    // move the iterator forward
-    if (zend_hash_move_forward_ex(_table, &_position) == SUCCESS)
-    {
-        // read current key and value
-        if (read()) return *this;
-        
-        // data was private or invalid, move further
-        return operator++();
-    }
-    else
-    {
-        // invalidate current position
-        invalidate();
-        
-        // done
-        return *this;
-    }
+    // done
+    return *this;
 }
 
 /**
@@ -73,98 +38,49 @@ ValueIterator &ValueIterator::operator++()
  */
 ValueIterator &ValueIterator::operator--()
 {
-    // leap out if we're not even iterating over a hash table
-    if (!_table) return *this;
+    // decrement implementation
+    _impl->decrement();
     
-    // if position is invalid, it is one position behind the last position
-    if (!_position)
-    {
-        // move to last position
-        zend_hash_internal_pointer_end_ex(_table, &_position);
-    }
-    else if (zend_hash_move_backwards_ex(_table, &_position) == FAILURE)
-    {
-        // invalidate current position
-        invalidate();
-        
-        // done
-        return *this;
-    }
-
-    // read current key and value
-    if (read()) return *this;
-    
-    // data was private, move on
-    return operator--();
+    // done
+    return *this;
 }
 
 /**
- *  Read current key and value
- *  @return bool        true if the object is in a valid position, false otherwise
+ *  Compare with other iterator
+ *  @param  that
+ *  @return bool
  */
-bool ValueIterator::read()
+bool ValueIterator::operator==(const ValueIterator &that) const
 {
-    // zval to read the current key in
-    Value key;
-
-#if PHP_VERSION_ID >= 50500
-
-    // read in the current key
-    zend_hash_get_current_key_zval_ex(_table, key._val, &_position);
-    
-    // if the key is set to NULL, it means that the object is not at a valid position
-    if (key.isNull()) return invalidate();
-    
-#else
-
-    // php 5.3 and php 5.4 need a different implementation because the function
-    // zend_hash_get_current_key_zval_ex is missing in php 5.3, declare variables
-    // we need for storing the key in
-    char *string_key;
-    unsigned int str_len;
-    unsigned long num_key;
-    
-    // get the current key
-    int type = zend_hash_get_current_key_ex(_table, &string_key, &str_len, &num_key, 0, &_position);
-    
-    // if key is not found, the iterator is at an invalid position
-    if (type == HASH_KEY_NON_EXISTANT) return invalidate();
-
-    // numeric keys are the easiest ones
-    if (type == HASH_KEY_IS_LONG) key = (int64_t)num_key;
-    else key = string_key;
-
-#endif
+    return _impl->equals(that._impl.get());
+}
 
-    // iterator is at a valid position, go fetch the data
-    // this is the variable we need for fetching the data
-    zval **value;
-            
-    // retrieve data
-    zend_hash_get_current_data_ex(_table, (void **) &value, &_position);
-    
-    // we can now update the current data
-    _current = std::make_pair<Value,Value>(std::move(key), *value);
+/**
+ *  Compare with other iterator
+ *  @param  that
+ *  @return bool
+ */
+bool ValueIterator::operator!=(const ValueIterator &that) const
+{
+    return !_impl->equals(that._impl.get());
+}
 
-    // if the key is private (it starts with a null character) we should return
-    // false to report that the object is not in a completely valid state
-    return !_current.first.isString() || _current.first.rawValue()[0];
+/**
+ *  Derefecence, this returns a std::pair with the current key and value
+ *  @return std::pair
+ */
+const std::pair<Value,Value> &ValueIterator::operator*() const
+{
+    return _impl->current();
 }
 
 /**
- *  Invalidate the iterator
- *  @return bool        always false
+ *  Dereference, this returns a std::pair with the current key and value
+ *  @return std::pair
  */
-bool ValueIterator::invalidate()
+const std::pair<Value,Value> *ValueIterator::operator->() const
 {
-    // forget current position
-    _position = nullptr;
-    
-    // make the data a pair of null ptrs
-    _current = std::make_pair<Value,Value>(nullptr,nullptr);
-    
-    // done
-    return false;
+    return &_impl->current();
 }
 
 /**