51

What is the shortest chunk of C++ you can come up with to safely clean up a std::vector or std::list of pointers? (assuming you have to call delete on the pointers?)

list<Foo*> foo_list;

I'd rather not use Boost or wrap my pointers with smart pointers.

Guy Avraham
  • 3,482
  • 3
  • 38
  • 50
twk
  • 16,760
  • 23
  • 73
  • 97
  • 2
    Smart pointers (including Boost::shared_ptr) will delete your objects under circumstances where you'll have a great deal of difficult seeing that it's done manually. – David Thornley Apr 19 '10 at 20:45
  • 2
    It's really dangerous to rely on code outside of the container to delete your pointers. What happens when the container is destroyed due to a thrown exception, for example? I know you said you don't like boost, but please consider the [boost pointer containers](http://www.boost.org/doc/libs/1_37_0/libs/ptr_container/doc/ptr_container.html). – Mark Ransom Nov 20 '08 at 23:14
  • 1
    One of the pitfalls is that, perversely, the STL allows several important iterator operations to throw exceptions. This makes many "obvious" approaches using iteration through a container unsafe. See http://stackoverflow.com/questions/7902452/may-stl-iterator-methods-throw-an-exception – Raedwald Oct 27 '11 at 12:21

15 Answers15

57

For std::list<T*> use:

while(!foo.empty()) delete foo.front(), foo.pop_front();

For std::vector<T*> use:

while(!bar.empty()) delete bar.back(), bar.pop_back();

Not sure why i took front instead of back for std::list above. I guess it's the feeling that it's faster. But actually both are constant time :). Anyway wrap it into a function and have fun:

template<typename Container>
void delete_them(Container& c) { while(!c.empty()) delete c.back(), c.pop_back(); }
Soup
  • 1,699
  • 15
  • 30
Johannes Schaub - litb
  • 496,577
  • 130
  • 894
  • 1,212
  • 2
    Technically correct, but it gets a lot longer if you use more common brace and indenting conventions. – Mark Ransom Nov 20 '08 at 23:10
  • read it from left to right: While foo is not empty, delete the front of foo, and pop the front of foo :p newlines would only get into the way :/ – Johannes Schaub - litb Nov 20 '08 at 23:26
  • 5
    I'd recommend against using the comma (sequence) operator. Too many C++ devs have no idea what it does, and will mistake it for a semicolon. – Kristopher Johnson Apr 19 '10 at 20:38
  • 2
    @Johannes Schaub - is it necessary to call delete. Isn't calling pop_back() sufficient? When I read what pop_back does , I see that it also calls the removed elements destructor. - http://www.cplusplus.com/reference/stl/vector/pop_back/. – Eternal Learner Aug 08 '11 at 17:03
  • 6
    @EternalLearner Calling the destructor doesn't de-allocate memory. You still need to call `delete` on the pointer so that the memory is de-allocated. – a_m0d Oct 06 '11 at 19:56
  • Why a comma and not a semicolon? (for readability, confusion) – Sandburg Sep 21 '18 at 15:08
  • @Sandburg not sure.. I guess, it did fit without scrollbars. With semicolons, I need braces around. Why do you think semicolons would cause confusion? – Johannes Schaub - litb Sep 22 '18 at 00:47
52

Since we are throwing down the gauntlet here... "Shortest chunk of C++"

static bool deleteAll( Foo * theElement ) { delete theElement; return true; }

foo_list . remove_if ( deleteAll );

I think we can trust the folks who came up with STL to have efficient algorithms. Why reinvent the wheel?

Mr.Ree
  • 8,320
  • 27
  • 30
  • I like it. I wrote a function template instead, but the remove_if idea is nice. – twk Nov 24 '08 at 18:03
  • 30
    Is it just me or does it feel really wrong when a Predicate have side effects? – Andre Rodrigues Dec 21 '11 at 17:02
  • C++ 11 and C++ 14 allows you to use lamdas for arguably easier reading. the code becomes: `foo_list.remove_if([](Foo *theElement){delete theElement; return true;}); ` – Matthew May 09 '17 at 20:08
31
for(list<Foo*>::const_iterator it = foo_list.begin(); it != foo_list.end(); ++it)
{
    delete *it;
} 
foo_list.clear();
Douglas Leeder
  • 52,368
  • 9
  • 94
  • 137
  • Upvoted because it performs well and is short. I added a modified version of your answer that is quite a bit shorter (but relies on C++11). – Adisak Feb 12 '14 at 22:06
  • The question did not specify const list. Why use const iterator ? – sp497 Sep 16 '14 at 11:35
  • 1
    @SRINI794 I used a const iterator to maximise for usability of the code, and because I wasn't going to alter the list during the iteration, so I didn't need a mutable iterator. – Douglas Leeder Apr 16 '15 at 07:29
  • @DouglasLeeder Dont want to be petty, yet usually the increment of the iterator is done by: ++it and not it++. Anyhow, it does not affect the correctness of the solution. – Guy Avraham Jan 30 '17 at 13:54
16

If you allow C++11, you can do a very short version of Douglas Leeder's answer:

for(auto &it:foo_list) delete it; foo_list.clear();
Adisak
  • 6,708
  • 38
  • 46
  • this is actually not a great idea for the simply reason that you are looping 2n times. once to loop through each element and deleting the memory then looping through the list again to clear the list and bring the list back down to zero. what you want to do is use an approach like Mr.Ree or Johannees Schaub's answer seen above. The both iterate over the list once performing both the deletion of memory and reducing of list size in one loop of size n. – Matthew May 09 '17 at 20:22
13

It's really dangerous to rely on code outside of the container to delete your pointers. What happens when the container is destroyed due to a thrown exception, for example?

I know you said you don't like boost, but please consider the boost pointer containers.

Mark Ransom
  • 299,747
  • 42
  • 398
  • 622
  • One of the pitfalls is that, perversely, the STL allows several important iterator operations to throw exceptions. This makes many "obvious" approaches using iteration through a container unsafe. See http://stackoverflow.com/questions/7902452/may-stl-iterator-methods-throw-an-exception – Raedwald Oct 27 '11 at 12:21
  • @YogeshArora Me too, but this doesn't make this a valid answer in any way. – Christian Rau Dec 05 '13 at 16:20
10
template< typename T >
struct delete_ptr : public std::unary_function<T,bool>
{
   bool operator()(T*pT) const { delete pT; return true; }
};

std::for_each(foo_list.begin(), foo_list.end(), delete_ptr<Foo>());
admo
  • 51
  • 1
  • 6
John Dibling
  • 99,718
  • 31
  • 186
  • 324
5

The following hack deletes the pointers when your list goes out of scope using RAII or if you call list::clear().

template <typename T>
class Deleter {
public:
  Deleter(T* pointer) : pointer_(pointer) { }
  Deleter(const Deleter& deleter) {
    Deleter* d = const_cast<Deleter*>(&deleter);
    pointer_ = d->pointer_;
    d->pointer_ = 0;
  }
  ~Deleter() { delete pointer_; }
  T* pointer_;
};

Example:

std::list<Deleter<Foo> > foo_list;
foo_list.push_back(new Foo());
foo_list.clear();
Linoliumz
  • 2,381
  • 3
  • 28
  • 35
  • I like that! It kind of works like the shared_ptr but for the container only and it is so minimal and elegant. I was looking at the boost smart pointers the other day and it is over 200K - incomprehensible - source code! – Dimitrios Menounos Aug 07 '12 at 11:58
  • That's really neat! If you overload the pointer dereference (*) and member selection (->) operator too you can make it entirely transparent I would have thought. – jsdw Apr 15 '13 at 23:55
5

I'm not sure that the functor approach wins for brevity here.

for( list<Foo*>::iterator i = foo_list.begin(); i != foo_list.end(); ++i )
    delete *i;

I'd usually advise against this, though. Wrapping the pointers in smart pointers or using a specialist pointer container is, in general, going to be more robust. There are lots of ways that items can be removed from a list ( various flavours of erase, clear, destruction of the list, assignment via an iterator into the list, etc. ). Can you guarantee to catch them all?

CB Bailey
  • 755,051
  • 104
  • 632
  • 656
  • The functor approach may not win for brevity, but that's not much of a prize. By using a functor you avoid having to write your own for loop, which is a source of many defects in software. – John Dibling Nov 21 '08 at 15:26
  • The question specifically asked about "shortest". I had no idea that manual for loops were a major source of defects, can you provide a reference? If a member of my team had a problem writing a bug free for loop, I'd rather not let him loose on a functor solution. – CB Bailey Nov 23 '08 at 11:55
  • The request was to "safely clean up" the container. But the safety of that code relies on several methods not throwing exceptions: `begin()`, `end()`, `iterator::operator!=`, `iterator::operator*`, `iterator::operator++`. Surpirsingly, such reliance is unsafe: http://stackoverflow.com/questions/7902452/may-stl-iterator-methods-throw-an-exception – Raedwald Oct 27 '11 at 12:25
4
for(list<Foo*>::const_iterator it = foo_list.begin(); it != foo_list.end(); it++)
{
    delete *it;
} 
foo_list.clear();

There's a small reason why you would not want to do this - you're effectively iterating over the list twice.

std::list<>::clear is linear in complexity; it removes and destroys one element at a time within a loop.

Taking the above into consideration the simplest to read solution in my opinion is:

while(!foo_list.empty())
{
    delete foo_list.front();
    foo_list.pop_front();
}
Heero
  • 41
  • 1
4

Actually, I believe the STD library provides a direct method of managing memory in the form of the allocator class

You can extend the basic allocator's deallocate() method to automatically delete the members of any container.

I /think/ this is the type of thing it's intended for.

Apurv
  • 3,723
  • 3
  • 30
  • 51
Jeremy Cowles
  • 1,171
  • 1
  • 9
  • 13
4

At least for a list, iterating and deleting, then calling clear at the end is a bit inneficient since it involves traversing the list twice, when you really only have to do it once. Here is a little better way:

for (list<Foo*>::iterator i = foo_list.begin(), e = foo_list.end(); i != e; )
{
    list<Foo*>::iterator tmp(i++);
    delete *tmp;
    foo_list.erase(tmp);
}

That said, your compiler may be smart enough to loop combine the two anyways, depending on how list::clear is implemented.

Greg Rogers
  • 35,641
  • 17
  • 67
  • 94
  • +1 because of the stability workaround. I think a big problem of the c++ is that this code more complex already as a such list manipulation in the plain old C. – peterh Feb 26 '14 at 10:49
3

Since C++11:

std::vector<Type*> v;
...
std::for_each(v.begin(), v.end(), std::default_delete<Type>());

Or, if you are writing templated code and want to avoid specifying a concrete type:

std::for_each(v.begin(), v.end(),
    std::default_delete<std::remove_pointer<decltype(v)::value_type>::type>());

Which (since C++14) can be shortened as:

std::for_each(v.begin(), v.end(),
    std::default_delete<std::remove_pointer_t<decltype(v)::value_type>>());
ostappus
  • 302
  • 2
  • 11
1
void remove(Foo* foo) { delete foo; }
....
for_each( foo_list.begin(), foo_list.end(), remove );
kendotwill
  • 1,892
  • 18
  • 17
0
for (list<Foo*>::const_iterator i = foo_list.begin(), e = foo_list.end(); i != e; ++i)
    delete *i;
foo_list.clear();
Assaf Lavie
  • 73,079
  • 34
  • 148
  • 203
0

This seems cleanest imo, but your c++ version must support this type of iteration (I believe anything including or ahead of c++0x will work):

for (Object *i : container) delete i;    
container.clear();
CompEng88
  • 1,336
  • 14
  • 25