-3

I have this class:

class Base{
     private:
        char *message;
     public:
        Base(string message`);
        ~Base();    
};

Edited: sorry, i forgot giving my constructor!
When i implement the destructor like below:

Base::Base(string message1){
     message = new char[message1.size() + 1]
     message[message1.size()] = '\0';
     memcpy(message, message1.c_str(), message.size());
}
Base::~Base(){
     delete message;  
}

sometime the system go wrong with stop working error, but if instead of delete message, i use message = NULL, everything will be alright! So, if I just declare message = NULL in my destructor, does my program get memory leak?

Kingfisher Phuoc
  • 8,052
  • 9
  • 46
  • 86

7 Answers7

10

There's absolutely no reason to use new here. The constructor takes a std::string and then manually copies its contents into a new dynamically allocated array of char. This is completely worthless. If instead, the constructor simply copied it into another std::string, the std::string copy constructor would do the same, but with lots of free benefits: exception-safety, no memory leaks, and proper copy semantics.

class Base{
     private:
        string message;
     public:
        Base(string message);
        // maybe a virtual destructor is desirable if this is a polymorphic base class
        // virtual ~Base() {}
};

Base::Base(string message1) : message(message1) {}
Community
  • 1
  • 1
R. Martinho Fernandes
  • 228,013
  • 71
  • 433
  • 510
5

You problem is using delete on something created with new[]. It needs a delete[] message to deallocate the string properly.

Setting the pointer to NULL just masks the problem, and does leak the memory.

Bo Persson
  • 90,663
  • 31
  • 146
  • 203
  • Bingo, and +1. This is the correct answer to the "sometime the system go wrong with stop working error" problem. Using `delete` rather than `delete[]` to release memory allocated via `new[]` is undefined behavior, and setting the pointer to NULL (and why bother?) leaks. – David Hammen Jun 11 '12 at 16:00
2

There is a memory leak. You allocate a char array, so you need to delete it appropriately, like this:

Base::~Base(){
     delete[] message;  
}

It is irrelevant if you set the pointer to NULL, the pointer itself is deallocated anyway after the destructor is called.

Eitan T
  • 32,660
  • 14
  • 72
  • 109
1

With the code shown here, there's no memory leak. But, affecting NULL to a pointer doesn't delete it in any case. You just loose the reference to it.

Marc Plano-Lesay
  • 6,808
  • 10
  • 44
  • 75
1

Why would you use delete? You aren't allocating memory on a heap, you are just creating a pointer. Unless, however you're allocating memory on the heap in the constructor, which we can't know considering you have chosen not to share the code...

fdh
  • 5,256
  • 13
  • 58
  • 101
1

Yes, it is a memory leak if you do not delete the memory dynamically allocated by new in your constructor.

You have to use delete[] message

Sanish
  • 1,699
  • 1
  • 12
  • 21
1

There is a memory leak because by simply doing message = NULL, you've only voided the address of the pointer. The contents that were held in memory still exist and have not been deleted.

user1084113
  • 932
  • 3
  • 15
  • 31