2

I just came across some container implementation in C++. That class uses an internal buffer to manage its objects. This is a simplified version without safety checks:

template <typename E> class Container
{
public:
   Container() : buffer(new E[100]), size(0) {}
   ~Container() { delete [] buffer; }

   void Add() { buffer[size] = E(); size++; }
   void Remove() { size--; buffer[size].~E(); }

private:
   E* buffer;
   int size;
};

AFAIK this will construct/destruct E objects redundantly in Container() and ~Container() if new/delete are not customized. This seems dangerous.

Is using placement new in Add() the best way to prevent dangerous redundant constructor / destructor calls (apart from binding the class to a fully featured pool)?

When using placement new, would new char[sizeof(E)*100] be the correct way for allocating the buffer?

Niall
  • 30,036
  • 10
  • 99
  • 142
Silicomancer
  • 8,604
  • 10
  • 63
  • 130

2 Answers2

5

AFAIK this will construct/destruct E objects redundantly

It would appear so. The newed array already applies the default constructor and the delete[] would call destructor as well for all the elements. In effect, the Add() and Remove() methods add little other than maintain the size counter.

When using placement new, would new char[sizeof(E)*100] be the correct way for allocating the buffer?

The best would be to opt for the std::allocator that handles all of the memory issues for you already.

Using a placement new and managing the memory yourself requires you to be aware of a number of issues (including);

  • Alignment
  • Allocated and used size
  • Destruction
  • Construction issues such as emplacement
  • Possible aliasing

None of these are impossible to surmount, it has just already been done in the standard library. If you are interested in pursuing a custom allocator, the global allocation functions (void* operator new (std::size_t count);) would be the appropriate starting point for the memory allocations.


Without further explanation on the original purpose of the code - a std::vector or a std::array would be far better options for managing the elements in the container.

Niall
  • 30,036
  • 10
  • 99
  • 142
  • 1
    I didn't know `std::allocator` gave stronger guarantees than a simple placement-new. Could you link to a reference somewhere? (cppreference, or section from standard) – Yam Marcovic Feb 01 '16 at 11:51
  • 1
    @YamMarcovic. It makes it easier because it already does all the hard work for you. – Niall Feb 01 '16 at 11:54
  • Yeah I'm just interested in the documentation of exactly what that hard work is, and whether it actually differs from placement-new. It's basically a documentation link I'm after (teach a man to fish... =] ) – Yam Marcovic Feb 01 '16 at 11:55
  • I see that standard containers are the best solution. But currently I need to understand the existing code. So would 'new char[sizeof(E)*100]' be the correct way of buffer allocation for placement new? – Silicomancer Feb 01 '16 at 11:57
  • @YamMarcovic. It is already linked in the post http://en.cppreference.com/w/cpp/memory/allocator. The standard allocator (generally) would use the placement `new` as well as the global allocation and deallocation functions. It is just that it is well tested and deals with the memory issues at play. – Niall Feb 01 '16 at 11:57
  • 1
    @Silicomancer. No, it would correct to use the global allocation function `::new (sizeof(E)*100);` – Niall Feb 01 '16 at 11:59
  • @Niall Which part of that page specifies what I asked about? Could you point to a particular paragraph or statement? – Yam Marcovic Feb 01 '16 at 12:00
  • @YamMarcovic. Um, I'm sure what you are after then. The construct method http://en.cppreference.com/w/cpp/memory/allocator/construct uses the placement new in it's implementation. – Niall Feb 01 '16 at 12:01
  • 1
    @Niall Oh, I see now. `::allocate()` calls `::new()` which guarantees proper alignment, and then `::construct()` will construct right at that aligned address using placement-new. Thanks for your patience. :) – Yam Marcovic Feb 01 '16 at 12:04
  • 2
    @YamMarcovic. No problem. Looking into the implementation of something like `vector` in your standard library implementation also offers some guidance on the use of `std::allocator` as well. – Niall Feb 01 '16 at 12:07
1

There's a number of issues with the code. If you call Remove() before calling Add() you will perform assignment to a destructed object.

Otherwise the delete[] buffer will call the destructor of 100 objects in the array. Which may have been called before.

Here's a valid program:

#include <iostream>

int counter=0;

class Example {

    public:
        Example():ID(++counter){
           std::cout<<"constructing "<<ID<<std::endl;
        }
        ~Example(){
            std::cout<<"destructing "<<ID<<std::endl;
            ID=-1;
        }
    private:


      int ID;
};

template <typename E> class Container
{
public:
   Container() : buffer(new char [100*sizeof(E)]), size(0) {}
   ~Container() {
        for(size_t i=0;i<size;++i){
            reinterpret_cast<E*>(buffer)[i].~E();
        }
        delete []  buffer; 
    }

   void Add() { new (buffer+sizeof(E)*size) E(); size++; }
   void Remove() { reinterpret_cast<E*>(buffer)[--size].~E(); }

private:
   void* buffer;
   size_t size;
};


int main() {
    Container<Example> empty;

    Container<Example> single;
    Container<Example> more;

    single.Add();

    more.Add();

    more.Remove();

    more.Add();
    more.Add();
    more.Remove();

    return 0;
}
Persixty
  • 8,165
  • 2
  • 13
  • 35