sparta::Buffer Enhancements (#527)

* First cut of Buffer::erase returning next itr

* Fixed bugs in tester for new erase

* Buffer::erase now returns a non-const iterator; trying to get reverse working

* Make sparta::Buffer::reverse_iterator behave more like an STL
reverse_iterator
    - rbegin() iterator now correctly dereferences to the entry
      before end()
    - Minor quirk: because end() is a constant, rbegin() will always be
      valid after an erase() or pop_back() as long as the Buffer is not
      empty

---------

Co-authored-by: Knute Lingaard <klingaard@gmail.com>

Author: bdutro

sparta/sparta/resources/Buffer.hpp

@@ -208,28 +208,32 @@ namespace sparta
             /// override the comparison operator.
             bool operator<(const BufferIterator& rhs) const
             {
-                sparta_assert(attached_buffer_ == rhs.attached_buffer_, "Cannot compare BufferIterators created by different buffers.");
+                sparta_assert(attached_buffer_ == rhs.attached_buffer_,
+                              "Cannot compare BufferIterators created by different buffers.");
                 return getIndex_() < rhs.getIndex_();
             }
 
             /// override the comparison operator.
             bool operator>(const BufferIterator& rhs) const
             {
-                sparta_assert(attached_buffer_ == rhs.attached_buffer_, "Cannot compare BufferIterators created by different buffers.");
+                sparta_assert(attached_buffer_ == rhs.attached_buffer_,
+                              "Cannot compare BufferIterators created by different buffers.");
                 return getIndex_() > rhs.getIndex_();
             }
 
             /// override the comparison operator.
             bool operator==(const BufferIterator& rhs) const
             {
-                sparta_assert(attached_buffer_ == rhs.attached_buffer_, "Cannot compare BufferIterators created by different buffers.");
+                sparta_assert(attached_buffer_ == rhs.attached_buffer_,
+                              "Cannot compare BufferIterators created by different buffers.");
                 return (buffer_entry_ == rhs.buffer_entry_);
             }
 
             /// override the not equal operator.
             bool operator!=(const BufferIterator& rhs) const
             {
-                sparta_assert(attached_buffer_ == rhs.attached_buffer_, "Cannot compare BufferIterators created by different buffers.");
+                sparta_assert(attached_buffer_ == rhs.attached_buffer_,
+                              "Cannot compare BufferIterators created by different buffers.");
                 return !operator==(rhs);
             }
 
@@ -245,39 +249,39 @@ namespace sparta
 
             /// override the dereferencing operator
             DataReferenceType operator* () const {
-                sparta_assert(attached_buffer_, "The iterator is not attached to a buffer. Was it initialized?");
+                sparta_assert(attached_buffer_,
+                              "The iterator is not attached to a buffer. Was it initialized?");
                 sparta_assert(isValid(), "Iterator is not valid for dereferencing");
                 return *(buffer_entry_->data);
             }
 
             //! Overload the class-member-access operator.
             value_type * operator -> () {
-                sparta_assert(attached_buffer_, "The iterator is not attached to a buffer. Was it initialized?");
+                sparta_assert(attached_buffer_,
+                              "The iterator is not attached to a buffer. Was it initialized?");
                 sparta_assert(isValid(), "Iterator is not valid for dereferencing");
                 return buffer_entry_->data;
             }
 
             value_type const * operator -> () const {
-                sparta_assert(attached_buffer_, "The iterator is not attached to a buffer. Was it initialized?");
+                sparta_assert(attached_buffer_,
+                              "The iterator is not attached to a buffer. Was it initialized?");
                 sparta_assert(isValid(), "Iterator is not valid for dereferencing");
                 return buffer_entry_->data;
             }
 
             /** brief Move the iterator forward to point to next element in queue ; PREFIX
              */
             BufferIterator & operator++() {
-                sparta_assert(attached_buffer_, "The iterator is not attached to a buffer. Was it initialized?");
-                if(isValid()) {
-                    uint32_t idx = buffer_entry_->physical_idx;
-                    ++idx;
-                    if(attached_buffer_->isValid(idx)) {
-                        buffer_entry_ = attached_buffer_->buffer_map_[idx];
-                    }
-                    else {
-                        buffer_entry_ = nullptr;
-                    }
-                } else {
-                    sparta_assert(attached_buffer_->numFree() > 0, "Incrementing the iterator to entry that is not valid");
+                sparta_assert(attached_buffer_,
+                              "The iterator is not attached to a buffer. Was it initialized?");
+                sparta_assert(isValid(), "Incrementing an iterator that is not valid");
+                const uint32_t idx = buffer_entry_->physical_idx + 1;
+                if(attached_buffer_->isValid(idx)) {
+                    buffer_entry_ = attached_buffer_->buffer_map_[idx];
+                }
+                else {
+                    buffer_entry_ = nullptr;
                 }
                 return *this;
             }
@@ -438,7 +442,7 @@ namespace sparta
          */
         const value_type & read(const const_reverse_iterator & entry) const
         {
-            return read(entry.base().getIndex_());
+            return read(std::prev(entry.base()));
         }
 
         /**
@@ -465,7 +469,7 @@ namespace sparta
          * \param entry the BufferIterator to read from.
          */
         value_type & access(const const_reverse_iterator & entry) {
-            return access(entry.base().getIndex_());
+            return access(std::prev(entry.base()));
         }
 
         /**
@@ -616,10 +620,11 @@ namespace sparta
          *          a BufferIterator has been created, the
          *          erase(BufferIterator&) should be used.
          */
-        void erase(const uint32_t& idx)
+        void erase(uint32_t idx)
         {
             // Make sure we are invalidating an already valid object.
-            sparta_assert(idx < size(), "Cannot erase an index that is not already valid");
+            sparta_assert(idx < size(),
+                          "Cannot erase an index that is not already valid");
 
             // Do the invalidation immediately
             // 1. Move the free space pointer to the erased position.
@@ -634,19 +639,18 @@ namespace sparta
             validator_->detachDataPointer(free_position_);
 
             // Shift all the positions above the invalidation in the map one space down.
-            uint32_t i = idx;
             sparta_assert(num_valid_ > 0);
             const uint32_t top_idx_of_buffer = num_valid_ - 1;
-            while(i < top_idx_of_buffer)
+            while(idx < top_idx_of_buffer)
             {
                 // assert that we are not going to do an invalid read.
-                sparta_assert(i + 1 < num_entries_);
-                buffer_map_[i] = buffer_map_[i + 1];
-                buffer_map_[i]->physical_idx = i;
+                sparta_assert(idx + 1 < num_entries_);
+                buffer_map_[idx] = buffer_map_[idx + 1];
+                buffer_map_[idx]->physical_idx = idx;
 
                 // Shift the indexes in the address map.
-                address_map_[i] = address_map_[i + 1];
-                ++i;
+                address_map_[idx] = address_map_[idx + 1];
+                ++idx;
             }
 
             // the entry at the old num_valid_ in the map now points to nullptr
@@ -664,22 +668,22 @@ namespace sparta
          * \brief erase the index at which the entry exists in the Buffer.
          * \param entry a reference to the entry to be erased.
          */
-        void erase(const const_iterator& entry)
+        iterator erase(const const_iterator& entry)
         {
-            sparta_assert(entry.attached_buffer_ == this, "Cannot erase an entry created by another Buffer");
+            sparta_assert(entry.attached_buffer_ == this,
+                          "Cannot erase an entry created by another Buffer");
             // erase the index in the actual buffer.
             erase(entry.getIndex_());
+            return {this, buffer_map_[entry.getIndex_()]};
         }
 
         /**
          * \brief erase the index at which the entry exists in the Buffer.
          * \param entry a reference to the entry to be erased.
          */
-        void erase(const const_reverse_iterator& entry)
+        reverse_iterator erase(const const_reverse_iterator& entry)
         {
-            sparta_assert(entry.base().attached_buffer_ == this, "Cannot erase an entry created by another Buffer");
-            // erase the index in the actual buffer.
-            erase(entry.base().getIndex_());
+            return reverse_iterator{erase(std::prev(entry.base()))};
         }
 
         /**
@@ -871,7 +875,8 @@ namespace sparta
         void resizeInternalContainers_() {
 
             // Assert that the Buffer class is in Infinite-Mode.
-            sparta_assert(is_infinite_mode_, "The Buffer class must be in Infinite-Mode in order to resize itself.");
+            sparta_assert(is_infinite_mode_,
+                          "The Buffer class must be in Infinite-Mode in order to resize itself.");
 
             // We do not resize if there are available slots in buffer.
             if(numFree() != 0) {
@@ -998,14 +1003,15 @@ namespace sparta
 
         std::string name_;
         const Clock * clk_ = nullptr;
-        size_type num_entries_;       /*!< The number of entries this buffer can hold */
-        PointerList buffer_map_; /*!< A vector list of pointers to all the items active in the buffer */
-        size_type data_pool_size_;    /*!< The number of elements our data_pool_ can hold*/
-        DataPool data_pool_; /*!< A vector twice the size of our Buffer size limit that is filled with pointers for our data.*/
-
-        DataPointer*  free_position_  = 0;  /*!< A pointer to a free position in our data_pool_ */
-        DataPointer*  first_position_  = 0; /*!< A pointer to a first position in our data_pool_; used for lower bound check */
-        size_type     num_valid_      = 0;  /*!< A tally of valid items */
+        size_type   num_entries_ = 0;    /*!< The number of entries this buffer can hold */
+        PointerList buffer_map_;         /*!< A vector list of pointers to all the items active in the buffer */
+        size_type   data_pool_size_ = 0; /*!< The number of elements our data_pool_ can hold*/
+        DataPool    data_pool_;          /*!< A vector twice the size of our Buffer size limit
+                                              that is filled with pointers for our data.*/
+
+        DataPointer*  free_position_  = nullptr; /*!< A pointer to a free position in our data_pool_ */
+        DataPointer*  first_position_ = nullptr; /*!< A pointer to a first position in our data_pool_; used for lower bound check */
+        size_type     num_valid_      = 0;       /*!< A tally of valid items */
         std::unique_ptr<DataPointerValidator> validator_;    /*!< Checks the validity of DataPointer */
 
         //////////////////////////////////////////////////////////////////////