3

I would like to ask if I's correct the following :

MyClass *obj = new MyClass();//allocate memory

obj.Variab="HELLO";

obj=NULL;
delete obj; //free memory

Is the memory allocated for obj deleted after the last two sentences? Appreciate.THX I would like to mention that I am working in c++ /Ubuntu. G++ is the compiler

EDIT:

What if I have?

int i=0;
list<string>l;
while (i<100)
{  
    MyClass *obj = new MyClass();//allocate memory
    obj->Variab="HELLO";
    //add the obj.valie in a list 
    l.push_back(obj);
    i=i+1;

delete obj; //free memory

}

it is ok?

JasonMArcher
  • 14,195
  • 22
  • 56
  • 52
sunset
  • 1,359
  • 5
  • 22
  • 35
  • `obj.Variab="HELLO"` will not work since you need to dereference the pointer first. Use `obj->Variab="HELLO"` or `(*obj).Variab="HELLO"` instead, where the first notation (called arrow notation) is much more conveniant. – Sammy S. Aug 26 '11 at 09:29
  • When you have additional questions, it makes sense to post them as single questions... – PlasmaHH Aug 26 '11 at 09:42
  • @sunset: do you want a `list` or a `list`? – amit Aug 26 '11 at 09:59

5 Answers5

4

no, you should use delete before assigning to NULL

delete obj; //free memory 
obj=NULL;

this is becasue the actual parameter to delete is the address of the allocated memory, but if you assign NULL before delete is used, you are actually passing NULL to delete, and nothing will happen, and you will get yourself a memory leak.


your edit question:

this code will not compile, as obj is not defined outside the while scope, in any case, also, l is a list<string> and you are trying to insert MyClass* types,this will result in another compilation error. also, you should use obj->Variab and not obj.Variab, since obj is a pointer.

EDIT to EDIT:

well, you still got a compilation error, since obj is not defined when you are trying to delete it. try this:

#include <iostream>
#include <list>
using namespace std;
class MyClass {
public:
    string Variab;
};

void myfunction (const string& s) {
  cout << " " << s;
}

int main()
{
    int i=0;
    list<string>l;
    while (i<100) {
        MyClass *obj = new MyClass();//allocate memory
        obj->Variab="HELLO";
        l.push_back(obj->Variab);
        i=i+1;
        delete obj; //free memory
    }
    for_each (l.begin(), l.end(), myfunction);

}
amit
  • 175,853
  • 27
  • 231
  • 333
  • it is important to put obj=NULL or it's optional?What is the difference? – sunset Aug 26 '11 at 09:19
  • @sunset: it's optional. some developers like that, as it is a strong indication that the memory is not in use, some avoid it as they see it unnecessery. – amit Aug 26 '11 at 09:20
  • one more question. I did change my question. Need an answet. THX A LOT!! – sunset Aug 26 '11 at 09:21
  • I consider it to be very helpful. You never know when the memory gets overriden, but until then you can possibly still use your object. This can lead to subtle and hard to find errors, so I'd make nullifying deleted pointers a convention in my code to make sure these errors never happen. – Sammy S. Aug 26 '11 at 09:24
  • 1
    It can be helpful to put `obj = NULL` (or `obj = 0`), because a) it allows you to check in your code if something was already deleted `if(obj != NULL) obj->Variab="Hello";` And b) when you call `delete obj` somewhere again, it does no damage. `delete NULL` doesn't do anything. – AudioDroid Aug 26 '11 at 09:25
  • WHAT ABOUT MY SECOND QUESTION? – sunset Aug 26 '11 at 09:26
  • @sunset: see my edit, your edit question will not compile. please next time if you want to ask a **DIFFERENT** question, create a new thread, instead of editting the last one – amit Aug 26 '11 at 09:28
  • @sunset: you put all the obj's in the list. If you flip the last two lines as suggested before, you delete the last obj that you put in the list. The rest of the obj's are still in the vector. You end up with one "broken" obj in your list. – AudioDroid Aug 26 '11 at 09:30
  • it was my mistake. I've changed . with ->. I know it's not correct. I don't know when to delete the pointer:) – sunset Aug 26 '11 at 09:32
  • @AudioDroid: so WHAT solution do you have? I know I am going to change just the last object? How to change my code? THX VERY MUCH. APPRECIATE – sunset Aug 26 '11 at 09:35
  • @sunset: you still should delete `obj` inside the `while` scope, otherwise you will get a compile error, since `obj` is not defined out of this scope. the memory allocated by the list will be taken care of automatically by `list`'s destructor, so you shouldn't worry about it. – amit Aug 26 '11 at 09:36
  • @amit. I've changed my code. When I want to print the liist the values are not correctly printed. The error consists in a lot of strange characters. WHY?How to solve this? – sunset Aug 26 '11 at 09:38
  • @sunset: 1) your list has to be `list ls;`, 2) you can set the `obj`-pointer to `NULL` after the while-loop, to make sure it's not pointing at anything anymore. 2) when you want to delete the objects in the list, you have to write a new while-loop: list::iterator iter = ls.begin(); while(iter!=ls.end()) { delete (*iter); ls.erase(iter);} – AudioDroid Aug 26 '11 at 09:47
  • @sunset: I just re-editted my answer, it works perfectly for me. try it out. I also added printing to the main function. [it assumes that you actually need a `list`. – amit Aug 26 '11 at 09:48
  • @amit: "delete `obj` inside the `while` scope"? What? Okay, you get rid of a compiler error. But who in the world would want code where you delete the `obj` in the while scope? I think you're getting "sunset" REALLY confused. – AudioDroid Aug 26 '11 at 09:48
  • @AudioDroid, yes. otherwise you will have a leak. notice that the list holds the **string**, and copies it by value [I assumed that what the OP wants, could be wrong about it of course]. – amit Aug 26 '11 at 09:50
  • @sunset: That code does not compile. Sorry. No way. You cannot pushback a `MyClass*`-object into a `list`. – AudioDroid Aug 26 '11 at 09:55
  • @amit: Some avoid it as we see it as dangerous and hides problems – Martin York Aug 26 '11 at 12:10
  • @AudioDroid: There are people that would argue that by hiding the double delete you are hiding a logical error in your code. By not setting it to NULL you are more likely to see a crash in testing and find the bug in your code and thus less likely to have real problems in production. – Martin York Aug 26 '11 at 12:13
4

This not correct:

obj = NULL; // don't do that! 
delete obj;

When you assign NULL to obj, you're losing the address it contained before, leaking the memory. When you then delete obj, you are deleting NULL, which is well-defined - as doing nothing.
As others have said,

delete obj;
obj = NULL;

is the common pattern to do that.

However, I consider it an anti-pattern.

  1. Whenever you are tempted to assign NULL to a pointer after deleting its content, ask yourself: Why is this pointer still in scope? Are you sure you still need it?
    It's much better to simply let a pointer fall out of scope once it's done.

  2. Whenever you are doing

    resource r = acquire();
    use(r);
    free(r);
    

    (with memory/dynamically allocated objects being the most common resource), alarm bells should go off. What if use(r) fails with an exception?
    Never use naked, dumb pointers. Better read about RAII and smart pointers.

Community
  • 1
  • 1
sbi
  • 219,715
  • 46
  • 258
  • 445
2

This would leak, delete will not clean up what you allocated with new. Change the order:

delete obj;
obj = NULL;  // I would avoid this.
Nim
  • 33,299
  • 2
  • 62
  • 101
0

Setting obj to null does not free the memory you allocated. The memory becomes no longer assigned to a variable but is still reserved, and results in a memory leak. Calling delete on a null pointer will have no effect. After the memory has been freed, the pointer becomes invalid and it is good practice to assign it to null. You need to switch the order of the last 2 statements:

delete obj; //free memory first
obj=NULL; //Set pointer to null for safety
EkcenierK
  • 1,429
  • 1
  • 19
  • 34
0

You have to delete the very same address as was returned by new - so you have to first delete, then set to null.

Setting pointer to null doesn't affect allocation - you just overwrite the address stored in the pointer and can't access the object anymore (which implies you can't delete the object and have it leaked).

sharptooth
  • 167,383
  • 100
  • 513
  • 979