From 744dd0afbbe064dc7dc3d92c77b9306560cbe742 Mon Sep 17 00:00:00 2001 From: Joshua Date: Wed, 30 May 2018 07:52:54 +1200 Subject: [PATCH 01/15] Remove uses of std::iterator (deprecated in C++17) Detailed reasoning for the deprecation is provided by the paper proposing deprecation (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0174r2.html) and a related LWG issue (https://cplusplus.github.io/LWG/issue2438). This was the only issue preventing a clean compile with VS 15.7.2 with c++17/c++latest set as the target language The issue could be resolved in two ways. Providing a custom replacement to std::iterator (a very simple structure) or by providing the 5 required typedefs. The only functional difference from my reading is that the typedefs are not immediately available to the implementer with the inheritance. I find the inline typedefs to be clearer hence the selection in this commit --- include/xlnt/workbook/worksheet_iterator.hpp | 33 +++++++++++------- include/xlnt/worksheet/cell_iterator.hpp | 36 ++++++++++++-------- include/xlnt/worksheet/range_iterator.hpp | 34 ++++++++++-------- 3 files changed, 61 insertions(+), 42 deletions(-) diff --git a/include/xlnt/workbook/worksheet_iterator.hpp b/include/xlnt/workbook/worksheet_iterator.hpp index d0ac56e7..b2830f88 100644 --- a/include/xlnt/workbook/worksheet_iterator.hpp +++ b/include/xlnt/workbook/worksheet_iterator.hpp @@ -37,18 +37,22 @@ class worksheet; // because one needs to point at a const workbook and the other needs // to point at a non-const workbook stored as a member variable, respectively. -/// -/// Alias the parent class of this iterator to increase clarity. -/// -using ws_iter_type = std::iterator; /// /// An iterator which is used to iterate over the worksheets in a workbook. /// -class XLNT_API worksheet_iterator : public ws_iter_type +class XLNT_API worksheet_iterator { public: + /// + /// iterator tags required for use with standard algorithms and adapters + /// + using iterator_category = std::bidirectional_iterator_tag; + using value_type = worksheet; + using difference_type = std::ptrdiff_t; + using pointer = worksheet *; + using reference = worksheet; // intentionally value + /// /// Constructs a worksheet iterator from a workbook and sheet index. /// @@ -106,18 +110,21 @@ private: }; -/// -/// Alias the parent class of this iterator to increase clarity. -/// -using c_ws_iter_type = std::iterator; - /// /// An iterator which is used to iterate over the worksheets in a const workbook. /// -class XLNT_API const_worksheet_iterator : public c_ws_iter_type +class XLNT_API const_worksheet_iterator { public: + /// + /// iterator tags required for use with standard algorithms and adapters + /// + using iterator_category = std::bidirectional_iterator_tag; + using value_type = const worksheet; + using difference_type = std::ptrdiff_t; + using pointer = const worksheet *; + using reference = const worksheet; // intentionally value + /// /// Constructs a worksheet iterator from a workbook and sheet index. /// diff --git a/include/xlnt/worksheet/cell_iterator.hpp b/include/xlnt/worksheet/cell_iterator.hpp index 496dd6a9..5f7b24eb 100644 --- a/include/xlnt/worksheet/cell_iterator.hpp +++ b/include/xlnt/worksheet/cell_iterator.hpp @@ -40,18 +40,21 @@ class cell; class cell_reference; class range_reference; -/// -/// Alias the parent class of this iterator to increase clarity. -/// -using c_iter_type = std::iterator; - /// /// A cell iterator iterates over a 1D range by row or by column. /// -class XLNT_API cell_iterator : public c_iter_type +class XLNT_API cell_iterator { public: + /// + /// iterator tags required for use with standard algorithms and adapters + /// + using iterator_category = std::bidirectional_iterator_tag; + using value_type = cell; + using difference_type = std::ptrdiff_t; + using pointer = cell *; + using reference = cell; // intentionally value + /// /// Constructs a cell_iterator from a worksheet, range, and iteration settings. /// @@ -135,7 +138,7 @@ private: /// If true, cells that don't exist in the worksheet will be skipped during iteration. /// bool skip_null_; - + /// /// If true, when on the last column, the cursor will continue to the next row /// (and vice versa when iterating in column-major order) until reaching the @@ -144,18 +147,21 @@ private: bool wrap_; }; -/// -/// Alias the parent class of this iterator to increase clarity. -/// -using cc_iter_type = std::iterator; - /// /// A cell iterator iterates over a 1D range by row or by column. /// -class XLNT_API const_cell_iterator : public cc_iter_type +class XLNT_API const_cell_iterator { public: + /// + /// iterator tags required for use with standard algorithms and adapters + /// + using iterator_category = std::bidirectional_iterator_tag; + using value_type = const cell; + using difference_type = std::ptrdiff_t; + using pointer = const cell *; + using reference = const cell; // intentionally value + /// /// Constructs a cell_iterator from a worksheet, range, and iteration settings. /// diff --git a/include/xlnt/worksheet/range_iterator.hpp b/include/xlnt/worksheet/range_iterator.hpp index cdd51d84..351e257f 100644 --- a/include/xlnt/worksheet/range_iterator.hpp +++ b/include/xlnt/worksheet/range_iterator.hpp @@ -35,19 +35,22 @@ namespace xlnt { class cell_vector; -/// -/// Alias the parent class of this iterator to increase clarity. -/// -using r_iter_type = std::iterator; - /// /// An iterator used by worksheet and range for traversing /// a 2D grid of cells by row/column then across that row/column. /// -class XLNT_API range_iterator : public r_iter_type +class XLNT_API range_iterator { public: + /// + /// iterator tags required for use with standard algorithms and adapters + /// + using iterator_category = std::bidirectional_iterator_tag; + using value_type = cell_vector; + using difference_type = std::ptrdiff_t; + using pointer = cell_vector *; + using reference = cell_vector; // intentionally value + /// /// Constructs a range iterator on a worksheet, cell pointing to the current /// row or column, range bounds, an order, and whether or not to skip null column/rows. @@ -127,19 +130,22 @@ private: bool skip_null_; }; -/// -/// Alias the parent class of this iterator to increase clarity. -/// -using cr_iter_type = std::iterator; - /// /// A const version of range_iterator which does not allow modification /// to the dereferenced cell_vector. /// -class XLNT_API const_range_iterator : public cr_iter_type +class XLNT_API const_range_iterator { public: + /// + /// this iterator meets the interface requirements of bidirection_iterator + /// + using iterator_category = std::bidirectional_iterator_tag; + using value_type = const cell_vector; + using difference_type = std::ptrdiff_t; + using pointer = const cell_vector *; + using reference = const cell_vector; // intentionally value + /// /// Constructs a range iterator on a worksheet, cell pointing to the current /// row or column, range bounds, an order, and whether or not to skip null column/rows. From 92ac3a2a2ea25731561cb91650a10f16147ef389 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Thu, 31 May 2018 13:07:02 +1200 Subject: [PATCH 02/15] cell_iterator - rule of 5/0 For rule of 5/0, where no implementation is required, all 5 operations have been declared as defaulted. This is less likely to forget definitions for all 5 if required - removed forwarding of copy to assignment (which was defaulted already) in favour of defaulted copy ctor - added defaulted move assignment/ctor and destructor --- include/xlnt/worksheet/cell_iterator.hpp | 37 +++++++++++++++++++++--- source/worksheet/cell_iterator.cpp | 10 ------- 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/include/xlnt/worksheet/cell_iterator.hpp b/include/xlnt/worksheet/cell_iterator.hpp index 5f7b24eb..64e5c9b2 100644 --- a/include/xlnt/worksheet/cell_iterator.hpp +++ b/include/xlnt/worksheet/cell_iterator.hpp @@ -64,12 +64,27 @@ public: /// /// Constructs a cell_iterator as a copy of an existing cell_iterator. /// - cell_iterator(const cell_iterator &other); + cell_iterator(const cell_iterator &) = default; /// /// Assigns this iterator to match the data in rhs. /// - cell_iterator &operator=(const cell_iterator &rhs) = default; + cell_iterator &operator=(const cell_iterator &) = default; + + /// + /// Constructs a cell_iterator by moving from a cell_iterator temporary + /// + cell_iterator(const cell_iterator &&) = default; + + /// + /// Assigns this iterator to from a cell_iterator temporary + /// + cell_iterator &operator=(const cell_iterator &&) = default; + + /// + /// destructor for const_cell_iterator + /// + ~cell_iterator() = default; /// /// Dereferences this iterator to return the cell it points to. @@ -169,15 +184,29 @@ public: const range_reference &limits, major_order order, bool skip_null, bool wrap); /// - /// 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. /// - const_cell_iterator(const const_cell_iterator &other); + const_cell_iterator(const const_cell_iterator &) = default; /// /// Assigns this iterator to match the data in rhs. /// const_cell_iterator &operator=(const const_cell_iterator &) = default; + /// + /// Constructs a const_cell_iterator by moving from a const_cell_iterator temporary + /// + const_cell_iterator(const const_cell_iterator &&) = default; + + /// + /// Assigns this iterator to from a const_cell_iterator temporary + /// + const_cell_iterator &operator=(const const_cell_iterator &&) = default; + + /// + /// destructor for const_cell_iterator + /// + ~const_cell_iterator() = default; /// /// Dereferences this iterator to return the cell it points to. /// diff --git a/source/worksheet/cell_iterator.cpp b/source/worksheet/cell_iterator.cpp index bad750ba..25aa7196 100644 --- a/source/worksheet/cell_iterator.cpp +++ b/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_ From 20e72d69e5aaf795a182b6e5fd94782b42734a88 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Thu, 31 May 2018 13:16:54 +1200 Subject: [PATCH 03/15] cell_iterator - operator* const overloads Users may still want to derederence a const iterator (note: not a const_iterator). Also use the "reference" typedef to ensure there is only 1 source of information --- include/xlnt/worksheet/cell_iterator.hpp | 14 +++++++++++++- source/worksheet/cell_iterator.cpp | 8 ++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/include/xlnt/worksheet/cell_iterator.hpp b/include/xlnt/worksheet/cell_iterator.hpp index 64e5c9b2..b11818f6 100644 --- a/include/xlnt/worksheet/cell_iterator.hpp +++ b/include/xlnt/worksheet/cell_iterator.hpp @@ -89,7 +89,13 @@ public: /// /// Dereferences this iterator to return the cell it points to. /// - cell operator*(); + reference operator*(); + + /// + /// Dereferences this iterator to return the cell it points to. + /// + const reference operator*() const; + /// /// Returns true if this iterator is equivalent to other. @@ -207,6 +213,12 @@ public: /// destructor for const_cell_iterator /// ~const_cell_iterator() = default; + + /// + /// Dereferences this iterator to return the cell it points to. + /// + const reference operator*() const; + /// /// Dereferences this iterator to return the cell it points to. /// diff --git a/source/worksheet/cell_iterator.cpp b/source/worksheet/cell_iterator.cpp index 25aa7196..e4c334e9 100644 --- a/source/worksheet/cell_iterator.cpp +++ b/source/worksheet/cell_iterator.cpp @@ -265,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 From 1c4f9c7ed2905722cf22dc1e85420fe5ec2143c9 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Thu, 31 May 2018 13:18:39 +1200 Subject: [PATCH 04/15] cell iterator - add operator-> Implementation just forwards to operator*, but this is a typical operator in the iterator API and is suprising and inconvenient that it is not present --- include/xlnt/worksheet/cell_iterator.hpp | 11 ++++++++++- source/worksheet/cell_iterator.cpp | 16 ++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/include/xlnt/worksheet/cell_iterator.hpp b/include/xlnt/worksheet/cell_iterator.hpp index b11818f6..9e179057 100644 --- a/include/xlnt/worksheet/cell_iterator.hpp +++ b/include/xlnt/worksheet/cell_iterator.hpp @@ -96,6 +96,15 @@ public: /// const reference operator*() const; + /// + /// Dereferences this iterator to return the cell it points to. + /// + reference operator->(); + + /// + /// Dereferences this iterator to return the cell it points to. + /// + const reference operator->() const; /// /// Returns true if this iterator is equivalent to other. @@ -222,7 +231,7 @@ public: /// /// Dereferences this iterator to return the cell it points to. /// - const cell operator*() const; + const reference operator->() const; /// /// Returns true if this iterator is equivalent to other. diff --git a/source/worksheet/cell_iterator.cpp b/source/worksheet/cell_iterator.cpp index e4c334e9..d1f66252 100644 --- a/source/worksheet/cell_iterator.cpp +++ b/source/worksheet/cell_iterator.cpp @@ -279,4 +279,20 @@ const const_cell_iterator::reference const_cell_iterator::operator*() const { return ws_.cell(cursor_); } + +cell_iterator::reference cell_iterator::operator->() +{ + return operator*(); +} + +const cell_iterator::reference cell_iterator::operator->() const +{ + return operator*(); +} + +const const_cell_iterator::reference const_cell_iterator::operator->() const +{ + return operator*(); +} + } // namespace xlnt From dc020622c055d5b1b2a2c5539d538ba8696ae385 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Thu, 31 May 2018 15:21:21 +1200 Subject: [PATCH 05/15] cell iterator - move parameters aren't const copy/paste error --- include/xlnt/worksheet/cell_iterator.hpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/xlnt/worksheet/cell_iterator.hpp b/include/xlnt/worksheet/cell_iterator.hpp index 9e179057..8dc53737 100644 --- a/include/xlnt/worksheet/cell_iterator.hpp +++ b/include/xlnt/worksheet/cell_iterator.hpp @@ -74,12 +74,12 @@ public: /// /// Constructs a cell_iterator by moving from a cell_iterator temporary /// - cell_iterator(const cell_iterator &&) = default; + cell_iterator(cell_iterator &&) = default; /// /// Assigns this iterator to from a cell_iterator temporary /// - cell_iterator &operator=(const cell_iterator &&) = default; + cell_iterator &operator=(cell_iterator &&) = default; /// /// destructor for const_cell_iterator @@ -211,12 +211,12 @@ public: /// /// Constructs a const_cell_iterator by moving from a const_cell_iterator temporary /// - const_cell_iterator(const const_cell_iterator &&) = default; + const_cell_iterator(const_cell_iterator &&) = default; /// /// Assigns this iterator to from a const_cell_iterator temporary /// - const_cell_iterator &operator=(const const_cell_iterator &&) = default; + const_cell_iterator &operator=(const_cell_iterator &&) = default; /// /// destructor for const_cell_iterator From 1e66824af06319a86f71555fd9476d15ed41479b Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Thu, 31 May 2018 15:27:20 +1200 Subject: [PATCH 06/15] range_iterator - rule of 5/0 For rule of 5/0, where no implementation is required, all 5 operations have been declared as defaulted. This is less likely to forget definitions for all 5 if required - removed forwarding of copy ctor to assignment (which was defaulted already) in favour of defaulted copy ctor - added defaulted move assignment/ctor and destructor --- include/xlnt/worksheet/range_iterator.hpp | 58 +++++++++++++++++------ source/worksheet/range_iterator.cpp | 10 ---- 2 files changed, 44 insertions(+), 24 deletions(-) diff --git a/include/xlnt/worksheet/range_iterator.hpp b/include/xlnt/worksheet/range_iterator.hpp index 351e257f..5e942f73 100644 --- a/include/xlnt/worksheet/range_iterator.hpp +++ b/include/xlnt/worksheet/range_iterator.hpp @@ -59,20 +59,35 @@ public: const range_reference &bounds, major_order order, bool skip_null); /// - /// Copy constructor. + /// Default copy constructor. /// - range_iterator(const range_iterator &other); - - /// - /// Dereference the iterator to return a column or row. - /// - cell_vector operator*() const; + range_iterator(const range_iterator &) = default; /// /// Default assignment operator. /// range_iterator &operator=(const range_iterator &) = default; + /// + /// Default move constructor. + /// + range_iterator(range_iterator &&) = default; + + /// + /// Default move assignment operator. + /// + range_iterator &operator=(range_iterator &&) = default; + + /// + /// Default destructor + /// + ~range_iterator() = default; + + /// + /// Dereference the iterator to return a column or row. + /// + cell_vector operator*() const; + /// /// Returns true if this iterator is equivalent to other. /// @@ -154,20 +169,35 @@ public: const range_reference &bounds, major_order order, bool skip_null); /// - /// Copy constructor. + /// Default copy constructor. /// - const_range_iterator(const const_range_iterator &other); - - /// - /// Dereferennce the iterator to return the current column/row. - /// - const cell_vector operator*() const; + const_range_iterator(const const_range_iterator &) = default; /// /// Default assignment operator. /// const_range_iterator &operator=(const const_range_iterator &) = default; + /// + /// Default move constructor. + /// + const_range_iterator(const_range_iterator &&) = default; + + /// + /// Default move assignment operator. + /// + const_range_iterator &operator=(const_range_iterator &&) = default; + + /// + /// Default destructor + /// + ~const_range_iterator() = default; + + /// + /// Dereferennce the iterator to return the current column/row. + /// + const cell_vector operator*() const; + /// /// Returns true if this iterator is equivalent to other. /// diff --git a/source/worksheet/range_iterator.cpp b/source/worksheet/range_iterator.cpp index 2da73d5c..57208d26 100644 --- a/source/worksheet/range_iterator.cpp +++ b/source/worksheet/range_iterator.cpp @@ -47,11 +47,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 +163,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_ From bc8cd21d676d1f08dfeec228407c6fca595e0088 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Thu, 31 May 2018 15:37:10 +1200 Subject: [PATCH 07/15] range_iterator - operator* const overloads Users may still want to dereference a const iterator (note: not a const_iterator). Also use the "reference" typedef to ensure there is only 1 source of information --- include/xlnt/worksheet/range_iterator.hpp | 9 +++++++-- source/worksheet/range_iterator.cpp | 9 +++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/include/xlnt/worksheet/range_iterator.hpp b/include/xlnt/worksheet/range_iterator.hpp index 5e942f73..c6f43979 100644 --- a/include/xlnt/worksheet/range_iterator.hpp +++ b/include/xlnt/worksheet/range_iterator.hpp @@ -86,7 +86,12 @@ public: /// /// Dereference the iterator to return a column or row. /// - cell_vector operator*() const; + reference operator*(); + + /// + /// Dereference the iterator to return a column or row. + /// + const reference operator*() const; /// /// Returns true if this iterator is equivalent to other. @@ -196,7 +201,7 @@ public: /// /// Dereferennce the iterator to return the current column/row. /// - const cell_vector operator*() const; + const reference operator*() const; /// /// Returns true if this iterator is equivalent to other. diff --git a/source/worksheet/range_iterator.cpp b/source/worksheet/range_iterator.cpp index 57208d26..2c7a3a10 100644 --- a/source/worksheet/range_iterator.cpp +++ b/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); } @@ -264,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); } From 62e0a7dd178ae5c1b45d2524a6a0c926b65c2035 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Thu, 31 May 2018 15:41:57 +1200 Subject: [PATCH 08/15] Revert "cell iterator - add operator->" This reverts commit 1b705ae043ae1b43d0ecbc93da04c944383bb296. --- include/xlnt/worksheet/cell_iterator.hpp | 11 +---------- source/worksheet/cell_iterator.cpp | 16 ---------------- 2 files changed, 1 insertion(+), 26 deletions(-) diff --git a/include/xlnt/worksheet/cell_iterator.hpp b/include/xlnt/worksheet/cell_iterator.hpp index 8dc53737..c6f1fa12 100644 --- a/include/xlnt/worksheet/cell_iterator.hpp +++ b/include/xlnt/worksheet/cell_iterator.hpp @@ -96,15 +96,6 @@ public: /// const reference operator*() const; - /// - /// Dereferences this iterator to return the cell it points to. - /// - reference operator->(); - - /// - /// Dereferences this iterator to return the cell it points to. - /// - const reference operator->() const; /// /// Returns true if this iterator is equivalent to other. @@ -231,7 +222,7 @@ public: /// /// Dereferences this iterator to return the cell it points to. /// - const reference operator->() const; + const cell operator*() const; /// /// Returns true if this iterator is equivalent to other. diff --git a/source/worksheet/cell_iterator.cpp b/source/worksheet/cell_iterator.cpp index d1f66252..e4c334e9 100644 --- a/source/worksheet/cell_iterator.cpp +++ b/source/worksheet/cell_iterator.cpp @@ -279,20 +279,4 @@ const const_cell_iterator::reference const_cell_iterator::operator*() const { return ws_.cell(cursor_); } - -cell_iterator::reference cell_iterator::operator->() -{ - return operator*(); -} - -const cell_iterator::reference cell_iterator::operator->() const -{ - return operator*(); -} - -const const_cell_iterator::reference const_cell_iterator::operator->() const -{ - return operator*(); -} - } // namespace xlnt From eaf3c2a773d26337c52194273c13bc673e80dc1c Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Thu, 31 May 2018 15:43:58 +1200 Subject: [PATCH 09/15] Remove double definition --- include/xlnt/worksheet/cell_iterator.hpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/include/xlnt/worksheet/cell_iterator.hpp b/include/xlnt/worksheet/cell_iterator.hpp index c6f1fa12..415dee6f 100644 --- a/include/xlnt/worksheet/cell_iterator.hpp +++ b/include/xlnt/worksheet/cell_iterator.hpp @@ -219,11 +219,6 @@ public: /// const reference operator*() const; - /// - /// Dereferences this iterator to return the cell it points to. - /// - const cell operator*() const; - /// /// Returns true if this iterator is equivalent to other. /// From a47985b2dbe76e624da8b0ef8a7e27542a849d92 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Thu, 31 May 2018 15:56:07 +1200 Subject: [PATCH 10/15] worksheet_iterator - rule of 5/0 For rule of 5/0, where no implementation is required, all 5 operations have been declared as defaulted. This is less likely to forget definitions for all 5 if required - removed forwarding of copy ctor to assignment (which was defaulted already) in favour of defaulted copy ctor - added defaulted move assignment/ctor and destructor Changed workbook reference to a pointer to allow tests to compile (reference isn't rebindable so defaulted assignment is equivalent to deleted) --- include/xlnt/workbook/worksheet_iterator.hpp | 42 +++++++++++++++++--- source/workbook/worksheet_iterator.cpp | 28 +++---------- 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/include/xlnt/workbook/worksheet_iterator.hpp b/include/xlnt/workbook/worksheet_iterator.hpp index b2830f88..c1b24a78 100644 --- a/include/xlnt/workbook/worksheet_iterator.hpp +++ b/include/xlnt/workbook/worksheet_iterator.hpp @@ -61,12 +61,27 @@ public: /// /// Copy constructs a worksheet iterator from another iterator. /// - worksheet_iterator(const worksheet_iterator &); + worksheet_iterator(const worksheet_iterator &) = default; /// /// Assigns the iterator so that it points to the same worksheet in the same workbook. /// - worksheet_iterator &operator=(const worksheet_iterator &); + worksheet_iterator &operator=(const worksheet_iterator &) = default; + + /// + /// Copy constructs a worksheet iterator from another iterator. + /// + worksheet_iterator(worksheet_iterator &&) = default; + + /// + /// Assigns the iterator so that it points to the same worksheet in the same workbook. + /// + worksheet_iterator &operator=(worksheet_iterator &&) = default; + + /// + /// Default destructor + /// + ~worksheet_iterator() = default; /// /// Dereferences the iterator to return the worksheet it is pointing to. @@ -101,7 +116,7 @@ private: /// /// The target workbook of this iterator. /// - workbook &wb_; + workbook *wb_; /// /// The index of the worksheet in wb_ this iterator is currently pointing to. @@ -133,12 +148,27 @@ public: /// /// Copy constructs a worksheet iterator from another iterator. /// - const_worksheet_iterator(const const_worksheet_iterator &); + const_worksheet_iterator(const const_worksheet_iterator &) = default; /// /// Assigns the iterator so that it points to the same worksheet in the same workbook. /// - const_worksheet_iterator &operator=(const const_worksheet_iterator &); + const_worksheet_iterator &operator=(const const_worksheet_iterator &) = default; + + /// + /// Move constructs a worksheet iterator from another iterator. + /// + const_worksheet_iterator(const_worksheet_iterator &&) = default; + + /// + /// Assigns the iterator from a temporary + /// + const_worksheet_iterator &operator=(const_worksheet_iterator &&) = default; + + /// + /// Default destructor + /// + ~const_worksheet_iterator() = default; /// /// Dereferences the iterator to return the worksheet it is pointing to. @@ -173,7 +203,7 @@ private: /// /// The target workbook of this iterator. /// - const workbook &wb_; + const workbook *wb_; /// /// The index of the worksheet in wb_ this iterator is currently pointing to. diff --git a/source/workbook/worksheet_iterator.cpp b/source/workbook/worksheet_iterator.cpp index daee7a64..0487cf33 100644 --- a/source/workbook/worksheet_iterator.cpp +++ b/source/workbook/worksheet_iterator.cpp @@ -28,18 +28,13 @@ namespace xlnt { worksheet_iterator::worksheet_iterator(workbook &wb, std::size_t index) - : wb_(wb), index_(index) -{ -} - -worksheet_iterator::worksheet_iterator(const worksheet_iterator &rhs) - : wb_(rhs.wb_), index_(rhs.index_) + : wb_(&wb), index_(index) { } worksheet worksheet_iterator::operator*() { - return wb_[index_]; + return (*wb_)[index_]; } worksheet_iterator &worksheet_iterator::operator++() @@ -50,7 +45,7 @@ worksheet_iterator &worksheet_iterator::operator++() worksheet_iterator worksheet_iterator::operator++(int) { - worksheet_iterator old(wb_, index_); + worksheet_iterator old(*wb_, index_); ++*this; return old; } @@ -65,25 +60,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) -{ -} - -const_worksheet_iterator::const_worksheet_iterator(const const_worksheet_iterator &rhs) - : wb_(rhs.wb_), index_(rhs.index_) + : wb_(&wb), index_(index) { } const worksheet const_worksheet_iterator::operator*() { - return wb_.sheet_by_index(index_); + return wb_->sheet_by_index(index_); } const_worksheet_iterator &const_worksheet_iterator::operator++() @@ -94,7 +78,7 @@ 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; } From 04b50d9b8ee5ee920f86209d3e972e8efcacd436 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Thu, 31 May 2018 16:00:40 +1200 Subject: [PATCH 11/15] worksheet_iterator - operator* const overloads Users may still want to dereference a const iterator (note: not a const_iterator). Also use the "reference" typedef to ensure there is only 1 source of information --- include/xlnt/workbook/worksheet_iterator.hpp | 11 +++++++++-- source/workbook/worksheet_iterator.cpp | 9 +++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/include/xlnt/workbook/worksheet_iterator.hpp b/include/xlnt/workbook/worksheet_iterator.hpp index c1b24a78..444b792f 100644 --- a/include/xlnt/workbook/worksheet_iterator.hpp +++ b/include/xlnt/workbook/worksheet_iterator.hpp @@ -88,7 +88,14 @@ public: /// If the iterator points to one-past-the-end of the workbook, an invalid_parameter /// exception will be thrown. /// - worksheet operator*(); + reference operator*(); + + /// + /// 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. + /// + const reference operator*() const; /// /// Returns true if this iterator points to the same worksheet as comparand. @@ -175,7 +182,7 @@ public: /// If the iterator points to one-past-the-end of the workbook, an invalid_parameter /// exception will be thrown. /// - const worksheet operator*(); + const reference operator*() const; /// /// Returns true if this iterator points to the same worksheet as comparand. diff --git a/source/workbook/worksheet_iterator.cpp b/source/workbook/worksheet_iterator.cpp index 0487cf33..3c981339 100644 --- a/source/workbook/worksheet_iterator.cpp +++ b/source/workbook/worksheet_iterator.cpp @@ -32,7 +32,12 @@ worksheet_iterator::worksheet_iterator(workbook &wb, std::size_t index) { } -worksheet worksheet_iterator::operator*() +worksheet_iterator::reference worksheet_iterator::operator*() +{ + return (*wb_)[index_]; +} + +const worksheet_iterator::reference worksheet_iterator::operator*() const { return (*wb_)[index_]; } @@ -65,7 +70,7 @@ const_worksheet_iterator::const_worksheet_iterator(const workbook &wb, std::size { } -const worksheet const_worksheet_iterator::operator*() +const const_worksheet_iterator::reference const_worksheet_iterator::operator*() const { return wb_->sheet_by_index(index_); } From b95919323d779f99096705520e48602933d113b3 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 9 Jun 2018 11:57:45 +1200 Subject: [PATCH 12/15] add more tests for worksheet iterator, add operator-- * operator-- completes the naive bidirectional iterator requirements * tests for various construction and assignment behaviours * tests for iteration and dereferencing behaviours --- include/xlnt/workbook/worksheet_iterator.hpp | 24 +++++++ source/workbook/worksheet_iterator.cpp | 26 +++++++ tests/workbook/workbook_test_suite.hpp | 76 ++++++++++++++++++-- 3 files changed, 122 insertions(+), 4 deletions(-) diff --git a/include/xlnt/workbook/worksheet_iterator.hpp b/include/xlnt/workbook/worksheet_iterator.hpp index 444b792f..f575dc7c 100644 --- a/include/xlnt/workbook/worksheet_iterator.hpp +++ b/include/xlnt/workbook/worksheet_iterator.hpp @@ -119,6 +119,18 @@ public: /// worksheet_iterator &operator++(); + /// + /// Post-decrement the iterator's internal workseet index. Returns a copy of the + /// iterator as it was before being incremented. + /// + worksheet_iterator operator--(int); + + /// + /// Pre-decrement the iterator's internal workseet index. Returns a refernce + /// to the same iterator. + /// + worksheet_iterator &operator--(); + private: /// /// The target workbook of this iterator. @@ -206,6 +218,18 @@ public: /// const_worksheet_iterator &operator++(); + /// + /// Post-decrement the iterator's internal workseet index. Returns a copy of the + /// iterator as it was before being incremented. + /// + const_worksheet_iterator operator--(int); + + /// + /// Pre-decrement the iterator's internal workseet index. Returns a refernce + /// to the same iterator. + /// + const_worksheet_iterator &operator--(); + private: /// /// The target workbook of this iterator. diff --git a/source/workbook/worksheet_iterator.cpp b/source/workbook/worksheet_iterator.cpp index 3c981339..b1432e33 100644 --- a/source/workbook/worksheet_iterator.cpp +++ b/source/workbook/worksheet_iterator.cpp @@ -55,6 +55,19 @@ worksheet_iterator worksheet_iterator::operator++(int) 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_; @@ -88,6 +101,19 @@ const_worksheet_iterator const_worksheet_iterator::operator++(int) 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_; diff --git a/tests/workbook/workbook_test_suite.hpp b/tests/workbook/workbook_test_suite.hpp index cbd4bda3..8a69a9cd 100644 --- a/tests/workbook/workbook_test_suite.hpp +++ b/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,84 @@ 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"); + --iter; + xlnt_assert_equals((*iter).title(), "Sheet1"); + std::advance(iter, 2); + xlnt_assert_equals(iter, wb2.end()); } - + + 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"); + --iter; + xlnt_assert_equals((*iter).title(), "Sheet1"); + std::advance(iter, 2); + xlnt_assert_equals(iter, wb2.cend()); + } + void test_get_index() { xlnt::workbook wb; From 9a82c4fab715f801173813e91c37d36863398438 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 9 Jun 2018 12:39:58 +1200 Subject: [PATCH 13/15] Comment fixes and clarifications --- include/xlnt/workbook/worksheet_iterator.hpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/include/xlnt/workbook/worksheet_iterator.hpp b/include/xlnt/workbook/worksheet_iterator.hpp index f575dc7c..3e0165fb 100644 --- a/include/xlnt/workbook/worksheet_iterator.hpp +++ b/include/xlnt/workbook/worksheet_iterator.hpp @@ -64,17 +64,17 @@ public: worksheet_iterator(const worksheet_iterator &) = default; /// - /// 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. /// worksheet_iterator &operator=(const worksheet_iterator &) = default; /// - /// Copy constructs a worksheet iterator from another iterator. + /// Move constructs a worksheet iterator from a temporary iterator. /// worksheet_iterator(worksheet_iterator &&) = default; /// - /// Assigns the iterator so that it points to the same worksheet in the same workbook. + /// Move assign the iterator from a temporary iterator /// worksheet_iterator &operator=(worksheet_iterator &&) = default; @@ -170,17 +170,17 @@ public: const_worksheet_iterator(const const_worksheet_iterator &) = default; /// - /// 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. /// const_worksheet_iterator &operator=(const const_worksheet_iterator &) = default; /// - /// Move constructs a worksheet iterator from another iterator. + /// Move constructs a worksheet iterator from a temporary iterator. /// const_worksheet_iterator(const_worksheet_iterator &&) = default; /// - /// Assigns the iterator from a temporary + /// Move assigns the iterator from a temporary iterator /// const_worksheet_iterator &operator=(const_worksheet_iterator &&) = default; From 4afc0963c6af750db1bb75c2f74372f772846282 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 9 Jun 2018 12:49:54 +1200 Subject: [PATCH 14/15] add post (in/de)crement tests --- tests/workbook/workbook_test_suite.hpp | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/tests/workbook/workbook_test_suite.hpp b/tests/workbook/workbook_test_suite.hpp index 8a69a9cd..531ca22c 100644 --- a/tests/workbook/workbook_test_suite.hpp +++ b/tests/workbook/workbook_test_suite.hpp @@ -193,10 +193,18 @@ public: ++iter; xlnt_assert_differs(iter, wb2.begin()); xlnt_assert_equals((*iter).title(), "Sheet2"); - --iter; + auto temp = --iter; + xlnt_assert_equals((*temp).title(), "Sheet1"); xlnt_assert_equals((*iter).title(), "Sheet1"); - std::advance(iter, 2); + 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() @@ -226,10 +234,18 @@ public: ++iter; xlnt_assert_differs(iter, wb2.cbegin()); xlnt_assert_equals((*iter).title(), "Sheet2"); - --iter; + auto temp = --iter; + xlnt_assert_equals((*temp).title(), "Sheet1"); xlnt_assert_equals((*iter).title(), "Sheet1"); - std::advance(iter, 2); + 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() From cffb1ce3452b9ce8738bd28b8d1153dea0996d00 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Tue, 10 Jul 2018 14:12:26 +1200 Subject: [PATCH 15/15] restart CI --- tests/workbook/workbook_test_suite.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/workbook/workbook_test_suite.hpp b/tests/workbook/workbook_test_suite.hpp index 531ca22c..e85a7a7e 100644 --- a/tests/workbook/workbook_test_suite.hpp +++ b/tests/workbook/workbook_test_suite.hpp @@ -227,8 +227,8 @@ public: iter = wb.cbegin(); iter = std::move(begin_2); xlnt_assert_equals(iter, wb2.cbegin()); - - wb2.create_sheet(); // wb2 iterators assumed invalidated + // wb2 iterators assumed invalidated + wb2.create_sheet(); iter = wb2.cbegin(); xlnt_assert_equals((*iter).title(), "Sheet1"); ++iter;