Am I deleting this properly?

Question

I have some struct:

struct A
{
 const char* name_;
 A* left_;
 A* right_;
 A(const char* name):name_(name),
      left_(nullptr),
      right_(nullptr){}
 A(const A&);
 //A(const A*);//ToDo
 A& operator=(const A&);
 ~A()
 {
  /*ToDo*/
 };
};
/*Just to compile*/
A& A::operator=(const A& pattern)
{

 //check for self-assignment
 if (this != &pattern) 
 {
  void* p = new char[sizeof(A)];
 }
 return *this;
}

A::A(const A& pat)
{
 void* p = new char[sizeof(A)];
 A* tmp = new (p) A("tmp");
 tmp->~A();
 delete tmp;//I WONDER IF HERE I SHOULD USE DIFFERENT delete[]?
}

int _tmain(int argc, _TCHAR* argv[])
{
 A a("a");
 A b = a;
 cin.get();
 return 0;
}

I know this is far from ideal and far from finished. But I would like to know if I'm deleting my memory in the proper way (please don't tell me how to do it properly. I'm trying to figure it out myself).

^{This is the link to different question which is really important to me.}

Answer 1

void* p = new char[sizeof(A)];
A* tmp = new (p) A("tmp");
tmp->~A();
delete tmp;//I WONDER IF HERE I SHOULD USE DIFFERENT delete[]?

No. You have already called the destructor so it is not correct to call delete which will cause another destructor call. You only need to free the memory. e.g.

delete[] static_cast<char*>(p);

If you are allocating raw memory for use with placement new it is more conventional to directly use an allocation function. e.g.

void* p = ::operator new[](sizeof(A));
A* tmp = new (p) A("tmp");
tmp->~A();
::operator delete[](p);

Consider doing something simpler, though. This block could be replaced with a single local variable which would be more robust.

A tmp("tmp");

Answer 2

If you allocate the memory with p = new[…], then you should deallocate with delete[] p^[1], no exceptions.

Don't mess with tmp after it's dead.

(Placement new doesn't allocate memory, the destructor won't modify the new char[sizeof(A)], so they don't enter to the question.)

^{[1]: You should declare p as a char*. Or cast p to a char* in delete[].}

Answer 3

If you are allocating memory for a pointer in the class, the destructor should deallocate (delete) the memory also:

class Node
{
  char * name_;
  Node * p_left;
  Node * p_right;

  Node(const char * new_name)
  : name_(NULL), p_left(NULL), p_right(NULL)
  {
    size_t size = strlen(new_name);
    name_ = new char [size + 1]; // + 1 for terminating null
  }
  ~Node()
  {
     delete[] name_;
  }
};

I highly suggest:

1. Using std::string instead of char * for text.
2. Move the links into a base class (preference is to use template for the data section of the Node.}

The base class will allow you to adapt the node to different data types:

struct Node
{
    Node * p_left;
    Node * p_right;
};

struct Name_Node
: public Node
{
  std::string name;
};

struct Integer_Node
: public Node
{
  int value;
};

OTOH, you may want to use std::map or std::list after this exercise.

Answer 4

To allocate memory for instance of A just write A* p = new A("tmp");. It will allocate memory and invoke constructor. Then use delete p; to invoke destructor and to deallocate memory. I can't see any need for using placement form of new in your case.

Answer 5

A::A(const A& pat)
{
 void* p = new char[sizeof(A)];
 A* tmp = new (p) A("tmp");
 tmp->~A();
 delete tmp;//I WONDER IF HERE I SHOULD USE DIFFERENT delete[]?
}

Yes, you should use delete[], since you created the memory with new[]. Furthermore, before you reach the constructor (in this case a copy constructor) the memory has already been allocated, so there's no need to allocate it again, which is what it appears you are trying to do here.

When you delete an object, the destructor (~A in this case) is called automatically, so there is no need to explicitly call it, unless you are using a placement new. So there is no need in the delete to explicitly remove the memory allocated for the object itself, only for the members that it owns.

The copy constructor should simply copy the important information out of the thing being copied. Last, the code here takes a string: A a("a");, so you'll need a string constructor to be able to make that call:

A::A(const std::string& name)
{
   //Do stuff
}