C++ moving a unique_ptr to a struct member

Question

I have the following program -

#include <iostream>
#include <memory>

class Person
{
   public:
      Person(const std::string& name):
         name(name) { }

      ~Person() { std::cout << "Destroyed" << std::endl; }

      std::string name;
};

typedef struct _container
{
   std::unique_ptr<Person> ptr;
}CONTAINER;

void func()
{
   CONTAINER* c = static_cast<CONTAINER*>(malloc(sizeof(CONTAINER)));
   std::unique_ptr<Person> p(new Person("FooBar"));
   c->ptr = std::move(p);
   std::cout << c->ptr->name << std::endl;
}


int main()
{
   func();
   getchar();

   return 0;
}

The program prints "FooBar". I expect the program to print "Destroyed" when func() return but it does not. Can someone help me with why that would not be happening in this case?

score 3 · Accepted Answer · answered Apr 06 '17 at 03:14

You've actually got undefined behaviour here. You cant just cast a malloc'd buffer to an object type. The constructors are never called and your member variables are in an invalid state.

You need to do either:

void func()
{
   CONTAINER c;
   std::unique_ptr<Person> p(new Person("FooBar"));
   c.ptr = std::move(p);
   std::cout << c.ptr->name << std::endl;
}

Or

void func()
{
   CONTAINER * c = new CONTAINER();
   std::unique_ptr<Person> p(new Person("FooBar"));
   c->ptr = std::move(p);
   std::cout << c->ptr->name << std::endl;
   delete c;
}

or if you really want to use malloc - you'll need to use placement new to get the right behaviour - but usually you dont want that, so I wont elaborate for now...

score 2 · Answer 2 · edited May 23 '17 at 12:17

You forget to add this line at the end of func().

delete c;

Here is the test (ideone).

c is a raw pointer. It is not a smart pointer. Thus, you have to delete it manually.

Deleting c will automatically delete CONTAINER::ptr because CONTAINER::ptr is a unique pointer.

However, you have malloc yourself, the more proper code might be :-

c->~_container();

Then free(), but I don't think it is needed in this case, because CONTAINER is not on a heap.
( I have never used malloc, so I am not sure about this part. )

Edit:
My solution is a quick patch to solve a single problem. (not print "Destroyed")
Please also read Michael Anderson's solution.
It addresses another underlying issue of the OP's code. (malloc)

Edit2: Here is a good link about placement new that Michael Anderson mentioned.
The code below is copied from the link (with little modification):-

int main(int argc, char* argv[]){
  const int NUMELEMENTS=20;
  char *pBuffer = new char[NUMELEMENTS*sizeof(A)];  
  //^^^ difference : your "CONTAINER" could be char[xxxx] (without new)
  A *pA = (A*)pBuffer;
  for(int i = 0; i < NUMELEMENTS; ++i) {
    pA[i] = new (pA + i) A();
  }
  printf("Buffer address: %x, Array address: %x\n", pBuffer, pA);
  // dont forget to destroy!
  for(int i = 0; i < NUMELEMENTS; ++i){
    pA[i].~A();
  }  
  delete[] pBuffer;//<--- no need to delete char[] if it is a stack variable
  return 0;
}

For further detail, please see that above link (because I don't want to copy more of it to here).

Here is another useful link : Using malloc in C++ is generally not recommended.

Ah, I see. Since the malloc'd area is not "smart", the program does not track 'c' the same way it tracks the scope of the unique pointer. — Jai Prabhu, Apr 06 '17 at 03:04
Nope - its worse than this, casting a malloc'd buffer to a type that has constructor is undefined behaviour - and the program is allowed to produce whatever garbage it likes. — Michael Anderson, Apr 06 '17 at 03:16

C++ moving a unique_ptr to a struct member

2 Answers2