6

The following code works as expected (the test passes) but I wonder if working with iterators in this way is considered a bad practice in c++ or if it is okay.

Maybe this is specific for std::vector and other collections behave differently and best practices vary between collections (or even their implementations)?

It certainly is not okay in other languages and most of the time changing a collection will invalidate iterators and throw exceptions.

BOOST_AUTO_TEST_CASE (ReverseIteratorExample) {
    std::vector<int> myvector;
    for(int i = 0; i < 5; i++)
    {
        myvector.push_back(i);
    }

    // is this generally a bad idea to change the vector while iterating?
    // is it okay in this specific case?
    myvector.reserve(myvector.size() + myvector.size() - 2 );
    myvector.insert(myvector.end(), myvector.rbegin() + 1, myvector.rend() -1);

    int resultset [8] = { 0,1,2,3,4,3,2,1 };
    std::vector<int> resultVector( resultset, resultset + sizeof(resultset)/sizeof(resultset[0]) );
    BOOST_CHECK_EQUAL_COLLECTIONS(myvector.begin(), myvector.end(), resultVector.begin(), resultVector.end());
}

Summarized Questions:

  1. Is this generally a bad idea to change the vector while iterating?
  2. Is it okay in this specific case?
  3. Is this specific for std::vector and other collections behave differently?
  4. Do best practices vary between collections (or even their implementations)?
tobsen
  • 5,228
  • 3
  • 30
  • 48
  • There is this really interesting SO question and its answers, to which I always refer to when in doubt: http://stackoverflow.com/questions/4114503/rules-for-iterator-invalidation It lists standard C++ containers, and the case where iterators are invalidated or still valid. (which answers your points 3. and 4.) – Stephane Rolland Apr 23 '13 at 17:08
  • Regarding #4: use `std::list` – TemplateRex Apr 23 '13 at 18:02

1 Answers1

12

This is not valid code. The standard's definition of operations on sequence containers states (23.2.3@4):

a.insert(p,i,j) - [...] pre: i and j are not iterators into a.

So your code invokes undefined behavior because it violates the precondition for the insert operation.

If instead of using insert, you wrote a loop iterating from myvector.rbegin() + 1 to myvector.rend() -1 and called push_back on all values, your code would be valid: This is because push_back only invalidates vector iterators if a reallocation is needed, and your call to reserve ensures that this is not the case.

In general, while there are some cases where modifying a container while iterating over it is fine (such as the loop described above), you have to make sure that your iterators aren't invalidated while doing so. When this happens is specific to each container.

interjay
  • 97,531
  • 20
  • 242
  • 238
  • for container specific iterator validation rules see this [question](http://stackoverflow.com/questions/6438086/iterator-invalidation-rules?lq=1) – TemplateRex Apr 23 '13 at 18:01
  • Thanks for your answer. Too bad that it seems to work though. I'll add it to my list of things I hate about c++. – tobsen Apr 24 '13 at 10:59