4

I am inheriting an interface, and implementing a virtual function that is supposed to do some work on a list of dynamically allocated objects. The first step is to remove duplicates from the list based on some custom equivalence criteria:

class Foo { /* ... */ };

struct FooLess
{
    bool operator()(const Foo *lhs, const Foo *rhs);
}

struct FooEqual
{
    bool operator()(const Foo *lhs, const Foo *rhs);
}

void doStuff(std::list<Foo*> &foos)
{
    // use the sort + unique idiom to find and erase duplicates
    FooLess less;
    FooEqual equal;
    foos.sort( foos.begin(), foos.end(), less );
    foos.erase( 
        std::unique( foos.begin(), foos.end(), equal ), 
        foos.end() ); // memory leak!
}

The problem is that using sort + unique doesn't clean up the memory, and the elements to be erased have unspecified values after unique, so I cannot perform the cleanup myself before eraseing. I was considering something like this:

void doStuff(std::list<Foo*> &foos)
{
    // make a temporary copy of the input as a list of auto_ptr's
    std::list<auto_ptr<Foo>> auto_foos;
    for (std::list<Foo>::iterator it = foos.begin(); it != foos.end(); ++it) 
        auto_foos.push_back(auto_ptr<Foo>(*it));
    foos.clear();

    FooLess less; // would need to change implementation to work on auto_ptr<Foo>
    FooEqual equal; // likewise
    auto_foos.sort( auto_foos.begin(), auto_foos.end(), less );
    auto_foos.erase( 
        std::unique( auto_foos.begin(), auto_foos.end(), equal ), 
        auto_foos.end() ); // okay now, duplicates deallocated

    // transfer ownership of the remaining objects back
    for (std::list<auto_ptr<Foo>>::iterator it = auto_foos.begin(); 
        it != auto_foos.end(); ++it) 
    { foos.push_back(it->get()); it->release(); }
}

Will this be okay, or am I missing something?

I am not able to use C++11 (Boost might be possible) or change the function signature to accept a list of straightforward Foos.

neuviemeporte
  • 6,310
  • 10
  • 49
  • 78
  • 2
    possible duplicate of [Why is it wrong to use std::auto\_ptr<> with standard containers?](http://stackoverflow.com/questions/111478/why-is-it-wrong-to-use-stdauto-ptr-with-standard-containers) – TNA Feb 06 '15 at 11:21
  • I read that answer, but I don't understand from it if using `auto_ptr` inside containers is wrong only if you expect for the original to be usable, or is it just wrong. – neuviemeporte Feb 06 '15 at 11:24

2 Answers2

2

To put an object into a standard container the object needs value semantics (the standard says "copy assignable" and "copy constructable"). Among other things, that means the copy constructor and assignment operator needs to create a copy of an object (leaving the original intact)

The auto_ptr copy constructor does not do that. Instead, the copy constructor and assignment operator transfer ownership of the pointer.

As a consequence, it is not possible for a standard container to contain an auto_ptr.

A lot of implementations (as in compiler and standard library) have the standard containers and/or auto_ptr coded so attempting to have a container of auto_ptr's will trigger a compiler error. Unfortunately, not all implementations do that.

Rob
  • 1,966
  • 9
  • 13
  • What if I am careful not to use the original after it has been copied? – neuviemeporte Feb 06 '15 at 11:18
  • 1
    Won't work. A lot of operations (resizing, adding or removing elements) rely on the elements behaving as expected. The "original" in that case is possibly still in the container, just waiting to be used. – Rob Feb 06 '15 at 11:26
  • 1
    @Lionel: no. The `unique_ptr` is exactly designed to be value type in containers. Unfortunately it's only available since C++11 on. – Ethouris Feb 06 '15 at 11:27
  • @neuviemeporte: won't work because, for example `std::unique` overwrites the original values, so your pointers, and the memory they point to, are lost (and the memory leaked) before you have a chance to delete. – Ethouris Feb 06 '15 at 11:28
  • @Ethouris: that's the reason I wanted to use auto_ptr – neuviemeporte Feb 06 '15 at 11:40
  • I know, but `auto_ptr` won't work either - it doesn't satisfy the `Copyable` concept, required by `std::list` and most of containers. – Ethouris Feb 06 '15 at 11:43
1

There are generally the following methods you can use in C++98:

  1. Define some pointer that will do what std::auto_ptr can't do. There was an old version of that thing, which contained an additional field of type bool that marked ownership. It was marked mutable, so it could be modified also in the object being read from when copying. The object was deleted at the end only if owned was true. Something like:

==

template <class T> class owning_ptr
{
   T* ptr;
   mutable bool owns;
   public:
   void operator =(T* src) { ptr = src; owns = true; }
   owning_ptr(const owning_ptr& other)
   {
       // copy the pointer, but STEAL ownership!
       ptr = other.ptr; owns = other.owns; other.owns = false;
   }
   T* release() { owns = false; return ptr; }
   ~owning_ptr() { if ( owns ) delete ptr; }
   /* ... some lacking stuff ..*/
 };
  1. You may try out boost::shared_ptr

  2. Instead of std::unique, you may try to do std::adjacent_find in a loop. Then you'll just find all elements that are "the same" as by your equal. If there's more than one element, you will erase them in place (you are allowed to do it because it's a list, so iterators remain valid).

Ethouris
  • 1,791
  • 13
  • 18
  • I went ahead with 3. Saves me the overhead of creating a copy of the input that I have to resync at the end. Thank you! – neuviemeporte Feb 06 '15 at 12:51