What's wrong with this method of deletion in a vector?

  • A+
Category:Languages

I have a deletion method that works and is as follows:

void deleteUserByID(int id, std::vector<Person*>& userList)  {      for(int i = 0; i < userList.size(); i++) {          if (userList.at(i)->getID() == id) {              userList.erase(userList.begin() + i);         }     } } 

However, I was trying the following before the above and couldn't understand why it wasn't working.

Instead of using userList.erase(userList.begin() + i);, I was using delete userList.at(i)

I'm somewhat new to C++, and have been instructed to delete heap allocated memory with the "delete" keyword. I felt that should have removed it from the Vector, but was wrong.

Why doesn't the delete userList.at(i) work? I'm curious. Any info would be helpful.

 


There are two separate concepts at play here. First, there's the maintenance of the std::vector that you're using. The vector's job is to hold a sequence of elements, and in many ways it doesn't really care what those elements actually are. From the vector's perspective, its elements will stick around until something explicitly comes along and says to get rid of them. The call to erase tells the vector "Hey, you know that element you've got at that one position? Please get rid of it." So when you make the call to erase, you're telling the vector to get rid of one of its elements.

Independently, there's the objects that are being stored in the vector. You're storing Person *s, which are pointers to Person objects. Those objects (I'm assuming) were allocated with new, so each Person essentially thinks "I'm going to live forever, or at least until someone comes around and calls delete on me." If you delete one of the Person objects, that object ceases to exist. However, the Person objects have absolutely no idea that there's a vector somewhere with pointers to people.

In order to get everything to work the way you want it to, you actually need to use a combination of both erase and delete (with a caveat that I'll mention later). If you just erase the pointers from the vector, then from the vector's perspective everything is cleaned up (it no longer holds pointers to the Person object in question), but from the Person's perspective the Person object is still very much alive and well because you never said to delete it. If you just delete the pointers, then from the Person's perspective everything is cleaned up (you've told the Person that it's time to go to the giant playground in the sky), but from the vector's perspective nothing was added or removed, so you now have a dangling pointer in your vector. In other words, the first option results in a memory leak - there's a Person object that was never told to clean itsefl up - and the second option results in dangling pointer - there's a pointer to what used to be a person, but which is now a bunch of bits that can be recycled however the program wishes.

Using the setup you have right now, the "best" way to handle this would be to use a combined approach. When you find an item to remove, first delete the pointer, then call erase. That ensures that the Person gets cleaned up and that the vector no longer has a dangling pointer in it.

But as some of the commenters have noted, there's a much better way to do this. Rather than storing Person *s and using raw pointers to reference the Person objects, use the std::shared_ptr type and manage your Person objects through std::shared_ptr<Person>. Unlike a regular pointer, which just says "yeah, there's a thing over there" and won't do any memory management on its own, the std::shared_ptr type actually owns the resource that it points at. If you erase a std::shared_ptr from a vector, the std::shared_ptr then says "okay, I just got kicked out of the vector, and if I'm the last pointer to the Person, I'll go and delete it for you." That means that you don't need to do any of your own memory management to clean things up.

In summary:

  • Just calling erase gets rid of an element from the vector, but leaves a Person adrift in the heap, wondering why no one loves it anymore.
  • Just calling delete sets the Person object free, but leaves a ghostly pointer to it in the vector that's a major hazard.
  • Calling both delete and erase in the proper order will solve this problem, but isn't the ideal solution.
  • Using std::shared_ptr instead of raw pointers is probably the best option, since it ensures that all the right deletes happen automatically.

Hope this helps!


And a quick addendum - are you sure that you code correctly visits all the elements of the vector? For example, if you erase the item at index 0, all the other elements of the vector will shift back one position. But then your implementation increments i to 1, at which point you've skipped over the item that just got shifted back to the first position.

I'll let you think about how to resolve this. Another answer has offered a good suggestion of using remove_if, which is one good solution, though if for your own edification you want to roll your own version, you might want to think over how you'd address the above issue.

Comment

:?: :razz: :sad: :evil: :!: :smile: :oops: :grin: :eek: :shock: :???: :cool: :lol: :mad: :twisted: :roll: :wink: :idea: :arrow: :neutral: :cry: :mrgreen: