Merge pull request #1 from Crzyrndm/dev-iterator-improvements

Dev: iterator improvements

include/xlnt/workbook/worksheet_iterator.hpp

@@ -61,19 +61,41 @@ public:
     /// <summary>
     /// Copy constructs a worksheet iterator from another iterator.
     /// </summary>
-    worksheet_iterator(const worksheet_iterator &);
+    worksheet_iterator(const worksheet_iterator &) = default;
 
     /// <summary>
-    /// Assigns the iterator so that it points to the same worksheet in the same workbook.
+    /// Copy assigns the iterator so that it points to the same worksheet in the same workbook.
     /// </summary>
-    worksheet_iterator &operator=(const worksheet_iterator &);
+    worksheet_iterator &operator=(const worksheet_iterator &) = default;
+
+    /// <summary>
+    /// Move constructs a worksheet iterator from a temporary iterator.
+    /// </summary>
+    worksheet_iterator(worksheet_iterator &&) = default;
+
+    /// <summary>
+    /// Move assign the iterator from a temporary iterator
+    /// </summary>
+    worksheet_iterator &operator=(worksheet_iterator &&) = default;
+
+    /// <summary>
+    /// Default destructor
+    /// </summary>
+    ~worksheet_iterator() = default;
 
     /// <summary>
     /// Dereferences the iterator to return the worksheet it is pointing to.
     /// If the iterator points to one-past-the-end of the workbook, an invalid_parameter
     /// exception will be thrown.
     /// </summary>
-    worksheet operator*();
+    reference operator*();
+
+    /// <summary>
+    /// Dereferences the iterator to return the worksheet it is pointing to.
+    /// If the iterator points to one-past-the-end of the workbook, an invalid_parameter
+    /// exception will be thrown.
+    /// </summary>
+    const reference operator*() const;
 
     /// <summary>
     /// Returns true if this iterator points to the same worksheet as comparand.
@@ -97,11 +119,23 @@ public:
     /// </summary>
     worksheet_iterator &operator++();
 
+    /// <summary>
+    /// Post-decrement the iterator's internal workseet index. Returns a copy of the
+    /// iterator as it was before being incremented.
+    /// </summary>
+    worksheet_iterator operator--(int);
+
+    /// <summary>
+    /// Pre-decrement the iterator's internal workseet index. Returns a refernce
+    /// to the same iterator.
+    /// </summary>
+    worksheet_iterator &operator--();
+
 private:
     /// <summary>
     /// The target workbook of this iterator.
     /// </summary>
-    workbook &wb_;
+    workbook *wb_;
 
     /// <summary>
     /// The index of the worksheet in wb_ this iterator is currently pointing to.
@@ -133,19 +167,34 @@ public:
     /// <summary>
     /// Copy constructs a worksheet iterator from another iterator.
     /// </summary>
-    const_worksheet_iterator(const const_worksheet_iterator &);
+    const_worksheet_iterator(const const_worksheet_iterator &) = default;
 
     /// <summary>
-    /// Assigns the iterator so that it points to the same worksheet in the same workbook.
+    /// Copy assigns the iterator so that it points to the same worksheet in the same workbook.
     /// </summary>
-    const_worksheet_iterator &operator=(const const_worksheet_iterator &);
+    const_worksheet_iterator &operator=(const const_worksheet_iterator &) = default;
+
+    /// <summary>
+    /// Move constructs a worksheet iterator from a temporary iterator.
+    /// </summary>
+    const_worksheet_iterator(const_worksheet_iterator &&) = default;
+
+    /// <summary>
+    /// Move assigns the iterator from a temporary iterator
+    /// </summary>
+    const_worksheet_iterator &operator=(const_worksheet_iterator &&) = default;
+
+    /// <summary>
+    /// Default destructor
+    /// </summary>
+    ~const_worksheet_iterator() = default;
 
     /// <summary>
     /// Dereferences the iterator to return the worksheet it is pointing to.
     /// If the iterator points to one-past-the-end of the workbook, an invalid_parameter
     /// exception will be thrown.
     /// </summary>
-    const worksheet operator*();
+    const reference operator*() const;
 
     /// <summary>
     /// Returns true if this iterator points to the same worksheet as comparand.
@@ -169,11 +218,23 @@ public:
     /// </summary>
     const_worksheet_iterator &operator++();
 
+    /// <summary>
+    /// Post-decrement the iterator's internal workseet index. Returns a copy of the
+    /// iterator as it was before being incremented.
+    /// </summary>
+    const_worksheet_iterator operator--(int);
+
+    /// <summary>
+    /// Pre-decrement the iterator's internal workseet index. Returns a refernce
+    /// to the same iterator.
+    /// </summary>
+    const_worksheet_iterator &operator--();
+
 private:
     /// <summary>
     /// The target workbook of this iterator.
     /// </summary>
-    const workbook &wb_;
+    const workbook *wb_;
 
     /// <summary>
     /// The index of the worksheet in wb_ this iterator is currently pointing to.

include/xlnt/worksheet/cell_iterator.hpp

@@ -64,17 +64,38 @@ public:
     /// <summary>
     /// Constructs a cell_iterator as a copy of an existing cell_iterator.
     /// </summary>
-    cell_iterator(const cell_iterator &other);
+    cell_iterator(const cell_iterator &) = default;
 
     /// <summary>
     /// Assigns this iterator to match the data in rhs.
     /// </summary>
-    cell_iterator &operator=(const cell_iterator &rhs) = default;
+    cell_iterator &operator=(const cell_iterator &) = default;
+
+    /// <summary>
+    /// Constructs a cell_iterator by moving from a cell_iterator temporary
+    /// </summary>
+    cell_iterator(cell_iterator &&) = default;
+
+    /// <summary>
+    /// Assigns this iterator to from a cell_iterator temporary
+    /// </summary>
+    cell_iterator &operator=(cell_iterator &&) = default;
+
+    /// <summary>
+    /// destructor for const_cell_iterator
+    /// </summary>
+    ~cell_iterator() = default;
 
     /// <summary>
     /// Dereferences this iterator to return the cell it points to.
     /// </summary>
-    cell operator*();
+    reference operator*();
+
+    /// <summary>
+    /// Dereferences this iterator to return the cell it points to.
+    /// </summary>
+    const reference operator*() const;
+
 
     /// <summary>
     /// Returns true if this iterator is equivalent to other.
@@ -169,19 +190,34 @@ public:
         const range_reference &limits, major_order order, bool skip_null, bool wrap);
 
     /// <summary>
-    /// Constructs a cell_iterator as a copy of an existing cell_iterator.
+    /// Constructs a const_cell_iterator as a copy of an existing cell_iterator.
     /// </summary>
-    const_cell_iterator(const const_cell_iterator &other);
+    const_cell_iterator(const const_cell_iterator &) = default;
 
     /// <summary>
     /// Assigns this iterator to match the data in rhs.
     /// </summary>
     const_cell_iterator &operator=(const const_cell_iterator &) = default;
 
+    /// <summary>
+    /// Constructs a const_cell_iterator by moving from a const_cell_iterator temporary
+    /// </summary>
+    const_cell_iterator(const_cell_iterator &&) = default;
+
+    /// <summary>
+    /// Assigns this iterator to from a const_cell_iterator temporary
+    /// </summary>
+    const_cell_iterator &operator=(const_cell_iterator &&) = default;
+
+    /// <summary>
+    /// destructor for const_cell_iterator
+    /// </summary>
+    ~const_cell_iterator() = default;
+
     /// <summary>
     /// Dereferences this iterator to return the cell it points to.
     /// </summary>
-    const cell operator*() const;
+    const reference operator*() const;
 
     /// <summary>
     /// Returns true if this iterator is equivalent to other.

include/xlnt/worksheet/range_iterator.hpp

@@ -59,20 +59,40 @@ public:
         const range_reference &bounds, major_order order, bool skip_null);
 
     /// <summary>
-    /// Copy constructor.
+    /// Default copy constructor.
     /// </summary>
-    range_iterator(const range_iterator &other);
+    range_iterator(const range_iterator &) = default;
-
-    /// <summary>
-    /// Dereference the iterator to return a column or row.
-    /// </summary>
-    cell_vector operator*() const;
 
     /// <summary>
     /// Default assignment operator.
     /// </summary>
     range_iterator &operator=(const range_iterator &) = default;
 
+    /// <summary>
+    /// Default move constructor.
+    /// </summary>
+    range_iterator(range_iterator &&) = default;
+
+    /// <summary>
+    /// Default move assignment operator.
+    /// </summary>
+    range_iterator &operator=(range_iterator &&) = default;
+
+    /// <summary>
+    /// Default destructor
+    /// </summary>
+    ~range_iterator() = default;
+
+    /// <summary>
+    /// Dereference the iterator to return a column or row.
+    /// </summary>
+    reference operator*();
+
+    /// <summary>
+    /// Dereference the iterator to return a column or row.
+    /// </summary>
+    const reference operator*() const;
+
     /// <summary>
     /// Returns true if this iterator is equivalent to other.
     /// </summary>
@@ -154,20 +174,35 @@ public:
         const range_reference &bounds, major_order order, bool skip_null);
 
     /// <summary>
-    /// Copy constructor.
+    /// Default copy constructor.
     /// </summary>
-    const_range_iterator(const const_range_iterator &other);
+    const_range_iterator(const const_range_iterator &) = default;
-
-    /// <summary>
-    /// Dereferennce the iterator to return the current column/row.
-    /// </summary>
-    const cell_vector operator*() const;
 
     /// <summary>
     /// Default assignment operator.
     /// </summary>
     const_range_iterator &operator=(const const_range_iterator &) = default;
 
+    /// <summary>
+    /// Default move constructor.
+    /// </summary>
+    const_range_iterator(const_range_iterator &&) = default;
+
+    /// <summary>
+    /// Default move assignment operator.
+    /// </summary>
+    const_range_iterator &operator=(const_range_iterator &&) = default;
+
+    /// <summary>
+    /// Default destructor
+    /// </summary>
+    ~const_range_iterator() = default;
+
+    /// <summary>
+    /// Dereferennce the iterator to return the current column/row.
+    /// </summary>
+    const reference operator*() const;
+
     /// <summary>
     /// Returns true if this iterator is equivalent to other.
     /// </summary>

source/workbook/worksheet_iterator.cpp

@@ -28,18 +28,18 @@
 namespace xlnt {
 
 worksheet_iterator::worksheet_iterator(workbook &wb, std::size_t index)
-    : wb_(wb), index_(index)
+    : wb_(&wb), index_(index)
 {
 }
 
-worksheet_iterator::worksheet_iterator(const worksheet_iterator &rhs)
+worksheet_iterator::reference worksheet_iterator::operator*()
-    : wb_(rhs.wb_), index_(rhs.index_)
 {
+    return (*wb_)[index_];
 }
 
-worksheet worksheet_iterator::operator*()
+const worksheet_iterator::reference worksheet_iterator::operator*() const
 {
-    return wb_[index_];
+    return (*wb_)[index_];
 }
 
 worksheet_iterator &worksheet_iterator::operator++()
@@ -50,11 +50,24 @@ worksheet_iterator &worksheet_iterator::operator++()
 
 worksheet_iterator worksheet_iterator::operator++(int)
 {
-    worksheet_iterator old(wb_, index_);
+    worksheet_iterator old(*wb_, index_);
     ++*this;
     return old;
 }
 
+worksheet_iterator &worksheet_iterator::operator--()
+{
+    --index_;
+    return *this;
+}
+
+worksheet_iterator worksheet_iterator::operator--(int)
+{
+    worksheet_iterator old(*wb_, index_);
+    --(*this);
+    return old;
+}
+
 bool worksheet_iterator::operator==(const worksheet_iterator &comparand) const
 {
     return index_ == comparand.index_ && wb_ == comparand.wb_;
@@ -65,25 +78,14 @@ bool worksheet_iterator::operator!=(const worksheet_iterator &comparand) const
     return !(*this == comparand);
 }
 
-worksheet_iterator &worksheet_iterator::operator=(const worksheet_iterator &other)
-{
-    index_ = other.index_;
-    return *this;
-}
-
 const_worksheet_iterator::const_worksheet_iterator(const workbook &wb, std::size_t index)
-    : wb_(wb), index_(index)
+    : wb_(&wb), index_(index)
 {
 }
 
-const_worksheet_iterator::const_worksheet_iterator(const const_worksheet_iterator &rhs)
+const const_worksheet_iterator::reference const_worksheet_iterator::operator*() const
-    : wb_(rhs.wb_), index_(rhs.index_)
 {
-}
+    return wb_->sheet_by_index(index_);
-
-const worksheet const_worksheet_iterator::operator*()
-{
-    return wb_.sheet_by_index(index_);
 }
 
 const_worksheet_iterator &const_worksheet_iterator::operator++()
@@ -94,11 +96,24 @@ const_worksheet_iterator &const_worksheet_iterator::operator++()
 
 const_worksheet_iterator const_worksheet_iterator::operator++(int)
 {
-    const_worksheet_iterator old(wb_, index_);
+    const_worksheet_iterator old(*wb_, index_);
     ++*this;
     return old;
 }
 
+const_worksheet_iterator &const_worksheet_iterator::operator--()
+{
+    --index_;
+    return *this;
+}
+
+const_worksheet_iterator const_worksheet_iterator::operator--(int)
+{
+    const_worksheet_iterator old(*wb_, index_);
+    --(*this);
+    return old;
+}
+
 bool const_worksheet_iterator::operator==(const const_worksheet_iterator &comparand) const
 {
     return index_ == comparand.index_ && wb_ == comparand.wb_;

source/worksheet/cell_iterator.cpp

@@ -58,16 +58,6 @@ const_cell_iterator::const_cell_iterator(worksheet ws, const cell_reference &cur
     }
 }
 
-cell_iterator::cell_iterator(const cell_iterator &other)
-{
-    *this = other;
-}
-
-const_cell_iterator::const_cell_iterator(const const_cell_iterator &other)
-{
-    *this = other;
-}
-
 bool cell_iterator::operator==(const cell_iterator &other) const
 {
     return ws_ == other.ws_
@@ -275,14 +265,18 @@ const_cell_iterator const_cell_iterator::operator++(int)
     return old;
 }
 
-cell cell_iterator::operator*()
+cell_iterator::reference cell_iterator::operator*()
 {
     return ws_.cell(cursor_);
 }
 
-const cell const_cell_iterator::operator*() const
+const cell_iterator::reference cell_iterator::operator*() const
 {
     return ws_.cell(cursor_);
 }
 
+const const_cell_iterator::reference const_cell_iterator::operator*() const
+{
+    return ws_.cell(cursor_);
+}
 } // namespace xlnt

source/worksheet/range_iterator.cpp

@@ -28,7 +28,12 @@
 
 namespace xlnt {
 
-cell_vector range_iterator::operator*() const
+range_iterator::reference range_iterator::operator*()
+{
+    return cell_vector(ws_, cursor_, bounds_, order_, skip_null_, false);
+}
+
+const range_iterator::reference range_iterator::operator*() const
 {
     return cell_vector(ws_, cursor_, bounds_, order_, skip_null_, false);
 }
@@ -47,11 +52,6 @@ range_iterator::range_iterator(worksheet &ws, const cell_reference &cursor,
     }
 }
 
-range_iterator::range_iterator(const range_iterator &other)
-{
-    *this = other;
-}
-
 bool range_iterator::operator==(const range_iterator &other) const
 {
     return ws_ == other.ws_
@@ -168,11 +168,6 @@ const_range_iterator::const_range_iterator(const worksheet &ws, const cell_refer
     }
 }
 
-const_range_iterator::const_range_iterator(const const_range_iterator &other)
-{
-    *this = other;
-}
-
 bool const_range_iterator::operator==(const const_range_iterator &other) const
 {
     return ws_ == other.ws_
@@ -274,7 +269,7 @@ const_range_iterator const_range_iterator::operator++(int)
     return old;
 }
 
-const cell_vector const_range_iterator::operator*() const
+const const_range_iterator::reference const_range_iterator::operator*() const
 {
     return cell_vector(ws_, cursor_, bounds_, order_, skip_null_, false);
 }

tests/workbook/workbook_test_suite.hpp

@@ -45,6 +45,7 @@ public:
         register_test(test_index_operator);
         register_test(test_contains);
         register_test(test_iter);
+        register_test(test_const_iter);
         register_test(test_get_index);
         register_test(test_get_sheet_names);
         register_test(test_add_named_range);
@@ -153,17 +154,100 @@ public:
         xlnt_assert(wb.contains("Sheet1"));
         xlnt_assert(!wb.contains("NotThere"));
     }
-    
+
     void test_iter()
     {
         xlnt::workbook wb;
-        
+
-        for(auto ws : wb)
+        for (auto ws : wb)
         {
             xlnt_assert_equals(ws.title(), "Sheet1");
         }
+
+        xlnt::workbook wb2;
+        auto iter = wb.begin();
+        auto iter_copy(iter); // copy ctor
+        xlnt_assert_equals(iter, iter_copy);
+        auto begin_2(wb2.begin());
+        xlnt_assert_differs(begin_2, iter);
+        iter = begin_2; // copy assign
+        xlnt_assert_equals(iter, begin_2);
+        iter = wb.begin();
+        iter = std::move(begin_2);
+        xlnt_assert_equals(iter, wb2.begin());
+
+        auto citer = wb.cbegin();
+        auto citer_copy(citer); // copy ctor
+        xlnt_assert_equals(citer, citer_copy);
+        auto cbegin_2(wb2.cbegin());
+        xlnt_assert_differs(cbegin_2, citer);
+        citer = cbegin_2; // copy assign
+        xlnt_assert_equals(citer, cbegin_2);
+        citer = wb.cbegin();
+        citer = std::move(cbegin_2);
+        xlnt_assert_equals(citer, wb2.cbegin());
+
+        wb2.create_sheet(); // wb2 iterators assumed invalidated
+        iter = wb2.begin();
+        xlnt_assert_equals((*iter).title(), "Sheet1");
+        ++iter;
+        xlnt_assert_differs(iter, wb2.begin());
+        xlnt_assert_equals((*iter).title(), "Sheet2");
+        auto temp = --iter;
+        xlnt_assert_equals((*temp).title(), "Sheet1");
+        xlnt_assert_equals((*iter).title(), "Sheet1");
+        iter++;
+        xlnt_assert_equals((*iter).title(), "Sheet2");
+        temp = iter++;
+        xlnt_assert_equals((*temp).title(), "Sheet2");
+        xlnt_assert_equals(iter, wb2.end());
+
+        iter = temp--;
+        xlnt_assert_equals((*iter).title(), "Sheet2");
+        xlnt_assert_equals((*temp).title(), "Sheet1");
     }
-    
+
+    void test_const_iter()
+    {
+        const xlnt::workbook wb;
+
+        for (auto ws : wb)
+        {
+            xlnt_assert_equals(ws.title(), "Sheet1");
+        }
+
+        xlnt::workbook wb2;
+        auto iter = wb.cbegin();
+        auto iter_copy(iter); // copy ctor
+        xlnt_assert_equals(iter, iter_copy);
+        auto begin_2(wb2.cbegin());
+        xlnt_assert_differs(begin_2, iter);
+        iter = begin_2; // copy assign
+        xlnt_assert_equals(iter, begin_2);
+        iter = wb.cbegin();
+        iter = std::move(begin_2);
+        xlnt_assert_equals(iter, wb2.cbegin());
+
+        wb2.create_sheet(); // wb2 iterators assumed invalidated
+        iter = wb2.cbegin();
+        xlnt_assert_equals((*iter).title(), "Sheet1");
+        ++iter;
+        xlnt_assert_differs(iter, wb2.cbegin());
+        xlnt_assert_equals((*iter).title(), "Sheet2");
+        auto temp = --iter;
+        xlnt_assert_equals((*temp).title(), "Sheet1");
+        xlnt_assert_equals((*iter).title(), "Sheet1");
+        iter++;
+        xlnt_assert_equals((*iter).title(), "Sheet2");
+        temp = iter++;
+        xlnt_assert_equals((*temp).title(), "Sheet2");
+        xlnt_assert_equals(iter, wb2.cend());
+
+        iter = temp--;
+        xlnt_assert_equals((*iter).title(), "Sheet2");
+        xlnt_assert_equals((*temp).title(), "Sheet1");
+    }
+
     void test_get_index()
     {
         xlnt::workbook wb;