1

I trying to to delete an object both from a vector of objects and from memory using its destructor. I understood the deleting the the object that the iterator points to, is making the the iterator to point to the element that follows the last element removed. Therefore, I tried to implement this:

std::vector<Customer*>::iterator j=customersList.begin();
while (j!=customersList.end()){
    customersList.erase(j);
    delete  *j;
}

is it o.k. or that it jumps 2 places by applying both erase and delete?

melpomene
  • 84,125
  • 8
  • 85
  • 148
Mr.O
  • 342
  • 2
  • 19
  • Why are you using pointers in the first place? `std::vector` allocates memory on the heap for you, you don't have to double that functionality. – Yksisarvinen Nov 04 '18 at 23:21
  • Your loop cannot be correct. To exit the loop, `j` must be `customersList.end()`, which means you just dereferenced an iterator past the end of the container in the last iteration of the loop (and tried to `delete` the invalid pointer). – melpomene Nov 04 '18 at 23:24
  • *it jumps 2 places* -- What do you mean by "jumps 2 places"? – PaulMcKenzie Nov 04 '18 at 23:25
  • @Yksisarvinen I don't really understand what you means, which functionality I double? – Mr.O Nov 04 '18 at 23:28
  • @Mr.OY -- Is there a reason to be using `std::vector` instead of `std::vector`? In any event, `j` becomes an invalid iterator, and then you're dereferencing the invalid iterator in the `delete`. Why not a simple `std::for_each`, deleting each element, and then a single `clear()` statement? – PaulMcKenzie Nov 04 '18 at 23:29
  • @melpomene this is exactly why asked this question, does both of erase and delete make the iterator move to the next element? – Mr.O Nov 04 '18 at 23:32
  • @Mr. OY. No, the iterator is not moved. And `delete` knows nothing about iterators or iterator "movement", only pointers. – PaulMcKenzie Nov 04 '18 at 23:33
  • @PaulMcKenzie for the first question: "one jump" =the iterator move to the next element. hopefully that will explain what I mean by jumps 2 places. for the second question: this is how I got the class, I think the reason is that the customers are in use in other places, and we don't want a copy of them. I will look up for what std::for_each saying because I don't know it. If you can explain the why of using it, it will wonderful. – Mr.O Nov 04 '18 at 23:39
  • @Mr.OY *the iterator move to the next element.* -- No, that is not correct. On *returrn* of the `erase()`, you get the next iterator. The `j` iterator does nothing. Look at [the documentation](https://en.cppreference.com/w/cpp/container/vector/erase) – PaulMcKenzie Nov 04 '18 at 23:40
  • @PaulMcKenzie please look at this answer: https://stackoverflow.com/a/2874533/9820561 It's saying that erase will make the iterator point to the next element. If I'm wrong, please tell me. – Mr.O Nov 04 '18 at 23:42
  • 1
    @Mr.OY -- You do see that your code fails to retrieve the return value of `erase`? Maybe you missed that very important part of that answer. – PaulMcKenzie Nov 04 '18 at 23:43
  • 1
    @Mr.OY It's not saying that at all. Where do you see that? – melpomene Nov 04 '18 at 23:47
  • I understand, thank you. – Mr.O Nov 04 '18 at 23:48
  • @melpomene yes I understood that I was wrong. – Mr.O Nov 04 '18 at 23:51

1 Answers1

3

The loop is not correct, since

  1. you are invalidating the j iterator, and subsequent to that, issuing a delete call on the dereferenced, invalid iterator.

  2. The j iterator is not incremented at all in that loop.

The easiest way to issue a delete and erase is a simple std::for_each, followed by a vector::clear().

#include <algorithm>
//...
std::for_each(std::begin(customersList), std::end(customersList), [](Customer *c){delete c;});
customersList.clear();

or even simply:

for (Customer* c : customersList )
    delete c;
customersList.clear();
PaulMcKenzie
  • 34,698
  • 4
  • 24
  • 45
  • 2
    I'd argue that a plain `for` loop is even easier: `for (Customer *c : customersList) { delete c; }` – melpomene Nov 04 '18 at 23:45
  • OK after understanding your code, I searched for some alternatives, tell me please if they are fine: first using for: `for ( it = customersList.begin(); it != customersList.end(); ) delete * it; it = customersList.erase(it); }` this is based on this answer: stackoverflow.com/a/991354/9820561 second try using while: `std::vector::iterator j=customersList.begin(); while (j!=customersList.end()){ delete *j; j=customersList.erase(j); }` – Mr.O Nov 05 '18 at 09:13