CODEWITHSUNDEEP
c++c++11containers
Bogdan Ionitza
Bogdan Ionitza

Reputation: 298

calling destructor in custom container

I am designing a custom C++ template container like this:

template <class C>
class Container {
public:
    ...
    void clear() {
        // should I call destructor on elements here?
        for (i...)
            array[i].~C(); // ?
    }

    ~Container() {
        delete [] array_;
    }
private:
    C* array_;
};

Should I call the destructor on elements manually inside clear()? If I don't, they will be called later when the container is destroyed (since I delete [] array_ in destructor), but this is not the expected behaviour (we expect them to be destroyed right inside clear(), just like std::vector does). However, if I do call the destructors, that memory remains in place and when I will be adding new elements on top of the old ones (which now are destroyed), the assignment operator will be called on those destroyed elements, and this may result in undefined behavior. Suppose I have a class:

class Foo {
public:
    Foo() { foo_ = new int; }
    ~Foo() { delete foo_; } // note I don't explicitly set foo_ to nullptr here
    Foo(Foo &&f) {
        f.foo_ = std::exchange(foo_, f.foo_); // standard practice in move constructors
    }
};

OK, this may look good so far, but now if I make a 

Container<Foo> c;
c.add(Foo());
c.clear();
c.add(Foo());

when calling clear() the destructor of the initial Foo is called, leaving its foo_ pointer dangling. Next, when adding the second Foo, the temporary R-value is exchanged with the old contents of the destructed object and when the temp will be destroyed, its destructor will try to delete the same dangling pointer again, which will crash.

So, how to correctly clear() a container without leaving room for double deletion? I also read that its recommended not to set pointers to nullptr in destructor in order not to not hide potential dangling pointer problems.

I'm a little confused about how to approach this.

EDIT:-------------------

There seems to be no compromise-free approach for a template container. So far I see these options, as others pointed out:

  1. completely delete the underlying array in clear(); this would be safe but not performant. I cannot use this option however since I'm implementing a lock-free multi-threaded container.
  2. instead of calling the destructor on elements in clear(), I would call the default constructor instead: array[i] = C(); This seems like the best solution so far - except it still implies an extra default construction.
  3. use placement new in add() - this seems to work for copy-construction. However, move-construction into an uninitialized object still seems problematic because most move constructors are implemented with exchanges between pointers - this would leave the source object (moved from) invalid.

Upvotes: 1

Views: 1595

Answers (2)

Necto
Necto

Reputation: 2654

As you specified in the comments, in the unassigned cells you have objects constructed by the default constructor (T()). That may be bad for performance, but certainly preserves the object abstraction. Instead of deleting the entries, just replace them with the ones constructed by the default constructor:

template <class C>
class Container {
public:
    ...
    void clear() {
        for (i...)
            array_[i] = C();
    }

    ~Container() {
        delete [] array_;
    }
private:
    C* array_;
};

Alternative implementation

On the other hand, I would propose a more performant approach, although it breaks the nice C++ abstraction. You could allocate the memory without invoking the unnecessary default constructor:

template <class C>
class Container {
public:
    Container(int n) {
        array_ = (C*)malloc(sizeof(C)*n);
        size_ = 0;
    }

    ...

    void clear() {
        for (... i < size_ ...)
            array_[i].~C();
        size_ = 0;
    }

    ~Container() {
        clear();
        free(array_);
    }
private:
    C* array_;
    int size_;
};

Here you do call destructors on all the initialized elements, but you do not call it second time, because you keep track of which are initialized, which are not.

Upvotes: 1

Peter
Peter

Reputation: 36597

The delete [] array_ will invoke the destructor for each element of array_ (assuming C is a type with a destructor).

Invoking a destructor twice on any given object gives undefined behaviour.

This means your clear() member function should not directly invoke destructors for the array elements.

If you insist on having a separate clear() function, which does not interfere with working of the destructor, simply implement it as

void clear()
{
    delete [] array_;
    array = nullptr;       //  NULL or 0 before C++11
}

This won't interfere with the destructor since operator delete has no effect if acting on a NULL pointer.

Upvotes: 3

Related Questions