🔒 Locked

rendundant check on delete.

Started by monkeyCode Sep 13, 2005 at 6:29 PM 6 replies 800+ views

Original Post

Sep 13, 2005 at 6:29 PM

I see alot of people still do this, i consider the following example redundant compared to the latter.


struct Data { ... };

class Object {
public:
  Object() : data(0) {
    data = new Data;
  }
  
  ~Object() {
    if(data) {
      delete data;
      data = 0;
    }
  }
  
protected:
  Data* data;
};

Because the standard guarantees that delete pointer will do nothing if pointer is equal to 0.


struct Data { ... };

class Object {
public:
  Object() : data(0) {
    data = new Data;
  }
  
  ~Object() {
    delete data;
  }
  
protected:
  Data* data;
};

Now for the reason i'm posting here, I simply dont trust my memory [rolleyes], haven't touched a line of code since may or so. So if anyone could clarify if the standard does indeed promise that no action will be taken if a pointer is 0, rather shure this holds true but better safe than sorry. Also if you use the first case, why? [grin] Edit: minor bugfixes [Edited by - monkeyCode on September 13, 2005 8:32:32 PM]

Richy2k

Sep 13, 2005 at 6:33 PM

Your second example is better practice - it ensures nothing will go wrong, and you can see that. Deleting a null pointer does nothing anyways, I believe your compiler will most likely optimise out your if statement. I *almost always* use protection with new, to make sure nothing has gone horribly wrong:

if (pointer == NULL){  pointer = new bleh;}else{  // oh poo}

monkeyCode

Sep 13, 2005 at 7:33 PM

still feels redundant tho, equivalent of writing

if(pointer) {	if(pointer {		pointer->action();	}}

just to be really shure that it is infact not 0.

Of course, in a larger scope i'd null out the pointer, but in this case, it's 0 on creation, any methods fiddling with it must leave it either valid or null, so why check the dtor?

Quote:

I believe your compiler will most likely optimise out your if statement

Might be, expect it to do so aswell, however i find the latter example in the OP cleaner.

Quote:

I *almost always* use protection with new, to make sure nothing has gone horribly wrong:

This is a rather specific scenario tho, exactly what could go wrong? null or valid.

SiCrane

Sep 13, 2005 at 7:38 PM

There's no reason to check the result of the new. Unless you use the nothrow form of new, and this isn't the case here, then new should throw an exception if there is a problem with the new expression. If you feel paranoid then put in an assertion. There's no need to clutter your release code with that kind of check.

Conner McCloud

Sep 13, 2005 at 7:39 PM

Quote:
Original post by Richy2k
I *almost always* use protection with new, to make sure nothing has gone horribly wrong:
if (pointer == NULL){  pointer = new bleh;}else{  // oh poo}

Under what circumstances would that help you?

CM

Evil Steve

Sep 13, 2005 at 7:50 PM

Quote:
Original post by Richy2k
Your second example is better practice - it ensures nothing will go wrong, and you can see that. Deleting a null pointer does nothing anyways, I believe your compiler will most likely optimise out your if statement. I *almost always* use protection with new, to make sure nothing has gone horribly wrong:
if (pointer == NULL){  pointer = new bleh;}else{  // oh poo}

That's what assert()ions are for [wink]

Deyja

Sep 13, 2005 at 8:38 PM

Object() : data(0) { //<- ew    data = new Data;  }Object() : data(new Data()) {} //<- better

Additionally, setting a member to 0 in the destructor is also pointless, as it's about to be destroyed anyway.

Andrew Russell

Sep 14, 2005 at 1:32 AM

While the trival example there is fine, it's probably better practice to do something like the following. Otherwise you're just asking for memory leaks if something else in the constructor throws (because the destructor is not called in that case).

class A{    Foo* data;public:    A() : data(NULL)    {        std::auto_ptr<Foo> adata(new Foo);        // stuff...        data = adata.release();    }    ~A() { delete data; }};

Then again, you should probably be making "data" an smart pointer in the first place...

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.