8

I have come across some code which has horrified me. Essentially it follows this pattern :

class Foo
{
  public:
    //default constructor
    Foo(): x(0), ptr(nullptr)  
    {
      //do nothing
    }

    //more interesting constructor
    Foo( FooInitialiser& init): x(0), ptr(nullptr) 
    {
      x = init.getX();
      ptr = new int;
    }
    ~Foo()
    {
      delete ptr;
    }

  private:
    int x;
    int* ptr;
};


void someFunction( FooInitialiser initialiser )
{
   int numFoos = MAGIC_NUMBER;
   Foo* fooArray = new Foo[numFoos];   //allocate an array of default constructed Foo's

   for(int i = 0; i < numFoos; ++i)
   {
       new( fooArray+ i) Foo( initialiser );    //use placement new to initialise
   }

    //... do stuff

   delete[] fooArray;
}

This code has been in the code base for years and it would seem has never caused a problem. It's obviously a bad idea since someone could change the default constructor to allocate not expecting the second construction. Simply replacing the second constructor with an equivalent initialisation method would seem the sensible thing to do. eg. 

void Foo::initialise(FooInitialiser& init)
{
    x = init.getX();
    ptr = new int;
}

Although still subject to possible resource leaks, at least a defensive programmer might think to check for prior allocations in a normal method.

My question is:

Is constructing twice like this actually undefined behaviour/ outlawed by standard or simply just a bad idea? If undefined behaviour can you quote or point me to right place to look in the standard?

David Woo
  • 749
  • 4
  • 13
  • did you try valgrind on this code? – zoska Dec 18 '14 at 17:37
  • The main problem I see is that `Foo` doesn't follow the rule of three -- the default copy-ctor and copy-assignment-operator won't do the right thing with `Foo::ptr`. – cdhowie Dec 18 '14 at 17:37
  • @cdhowie Maybe we should not assume the worst about other people's code. I guess OP simply cut out the code that was not necessary to ask the question. – anatolyg Dec 18 '14 at 18:50
  • @cdhowie anatolyg is right. The original code has deleted copy constructor and assignment operator. I didn't think it relevant to my question. – David Woo Dec 18 '14 at 19:40
  • @DavidWoo Fair point. Just thought I'd point it out in case these members were indeed default-implemented. – cdhowie Dec 18 '14 at 19:43
  • @MattMcNabb Yes - Sorry that should not be there. I was oriignally going to expand the constructor out of class. Ill edit – David Woo Dec 22 '14 at 11:12

1 Answers1

12

Generally, working with placement new in this way is not a good idea. Calling an initializer from the first new, or calling an initializer instead of placement new are both considered to be better form than the code you've provided.

However, in this case, the behaviour of calling placement new over an existing object is well defined.

A program may end the lifetime of any object by reusing the storage which the object occupies or by explicitly calling the destructor for an object of a class type with a non-trivial destructor. For an object of a class type with a non-trivial destructor, the program is not required to call the destructor explicitly before the storage which the object occupies is reused or released; however, if there is no explicit call to the destructor or if a delete-expression (5.3.5) is not used to release the storage, the destructor shall not be implicitly called and any program that depends on the side effects produced by the destructor has undefined behavior.

So when this happens:

Foo* fooArray = new Foo[numFoos];   //allocate an array of default constructed Foo's

for(int i = 0; i < numFoos; ++i)
{
    new( fooArray+ i) Foo( initialiser );    //use placement new to initialise
}

The placement new operation will end the lifetime of the Foo that was there, and create a new one in it's place. In many circumstances this could be bad, but given the way your destructor works, this will be fine.

Calling placement new on an existing object could be undefined behaviour, but it depends on the specific object.

This does not produce undefined behaviour, because you are not depending on the "side effects" produced by the destructor.

The only "side-effect" in the destructor of your object is to delete the contained int pointer, but in this case that object is never in a deletable state when placement new is called.

If it was possible for the contained int pointer to be equal to something other than nullptr and could possibly require deleting, then calling placement new over the existing object would invoke undefined behaviour.

Michael Gazonda
  • 2,720
  • 1
  • 17
  • 33