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?
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.
So when this happens:
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 containedint
pointer, but in this case that object is never in a deletable state when placementnew
is called.If it was possible for the contained
int
pointer to be equal to something other thannullptr
and could possibly require deleting, then calling placementnew
over the existing object would invoke undefined behaviour.