pointers and references question

Question

#ifndef DELETE
    #define DELETE(var) delete var, var = NULL
#endif

using namespace std;

class Teste {
    private:
        Teste *_Z;

    public:
    Teste(){
        AnyNum = 5;
        _Z = NULL;
    }
    ~Teste(){
        if (_Z != NULL)
            DELETE(_Z);
    }

    Teste *Z(){
        _Z = new Teste;
        return _Z;
    }
    void Z(Teste *value){
        value->AnyNum = 100;
        *_Z = *value;
    }

    int AnyNum;
};

int main(int argc, char *argv[]){
    Teste *b = new Teste, *a;

    a = b->Z();

    cout << "a->AnyNum: " << a->AnyNum << "\n";

    b->Z(new Teste);

    cout << "a->AnyNum: " << a->AnyNum << "\n";

    //wdDELETE(a);
    DELETE(b);
    return 0;
}

I would like to know if there is a memory leak in this code it works ok, the *a is set twice and the AnyNum prints different numbers on each cout << but I wonder what happened to the _Z after the setter(new Teste), I don't have much knowledge in pointers/references yet, but for the logic I guess it is being swapped for the new variable if it is leaking, is there anyway to accomplish this without having to set a to _Z again? because the address didn't change, just the direct memory allocated I was going to use *& instead of just pointers, but would it make difference?

Answer 1

There is a memory leak on this line:

b->Z(new Teste);

because of the definition of the function:

void Z(Teste *value){
    value->AnyNum = 100;
    *_Z = *value;
}

It looks like Z without arguments was supposed to be a getter and with arguments a setter. I suspect you meant to do:

void Z(Teste *value){
    value->AnyNum = 100;
    _Z = value;
}

(note the third line) That is, assign the pointer "value" to the pointer "_Z" instead of copy what value pointed at over what Z pointed at. With that, the first memory leak would be resolved, but the code would still have one since _Z could have been holding a pointer. So you'd have to do:

void Z(Teste *value){
    value->AnyNum = 100;
    delete _Z; // you don't have to check for null
    _Z = value;
}

As mentioned in another comment, the real solution is to use smart pointers. Here's a more modern approach to the same code:

using namespace std;

class Teste {
    private:
        boost::shared_ptr<Teste> Z_;

    public:
    Teste() : AnyNum(5), Z_(NULL)
    { }

    boost::shared_ptr<Teste> Z()
    {
        Z_.reset(new Teste);
        return Z_;
    }

    void Z(boost::shared_ptr<Teste> value)
    {
        value->AnyNum = 100;
        Z_ = value;
    }

    int AnyNum;
};

int main(int argc, char *argv[]){
    boost::shared_ptr<Teste> b = new Teste, a;

    a = b->Z();

    cout << "a->AnyNum: " << a->AnyNum << "\n";

    b->Z(boost::shared_ptr<Teste>(new Teste));

    cout << "a->AnyNum: " << a->AnyNum << "\n";

    return 0;
}

Answer 2

Yes there is:

void Z(Teste *value)
{
   value->AnyNum = 100;
   *_Z = *value; // you need assignment operator
}

The compiler-generated assignment operator will not make a deep copy, instead it will make a shallow copy. What you have to do is to write a suitable assignment operator (and possibly a copy constructor) for Teste. Also, you don't have to check if a pointer is NULL before deleting it:

~Teste()
{
   // no need for checking. Nothing will happen if you delete a NULL pointer
   if (_Z != NULL)
     DELETE(_Z);
}

Answer 3

You've got another problem: _Z is not an identifier you should be using. In general, it's best to avoid leading underscores, and in particular double underscores or underscores followed by a capital letter are reserved for the implementation.

Answer 4

What a mess! The whole program is very hard to read because of the choice of identifier names to start with:

#ifndef DELETE
    #define DELETE(var) delete var, var = NULL
#endif

I find that very ugly. When using classes it seems very un-nessacery. You could use it where a variables is going out of scope but it is a waster of time in the destructor. I think it would be asier to wrap the code in some smart pointer:

---

class Teste
{
    private:
        Teste *_Z;

    public:
        Teste()
        ~Teste()    // Delete the _Z pointer.
        Teste *Z();
        void Z(Teste *value);
};

Ok. You have a pointer member that you delete in the destructor. This means you are taking ownership of the pointer. This means that the ule of four applies (similar to the rule of three but applicable to ownership rules). This means you basically need to write 4 methods or the compiler generated versions will mess up your code. The methods you should write are:

A Normal (or default constructor)
A Copy constructor
An Assignment operator
A destructor.

Your code only has two of these. You need to write the other two. Or your object should not take ownership of the RAW pointer. ie. use a Smart Pointer.

---

Teste *_Z;

This is not allowed. Identifiers beginning with an underscore and a capitol letter are reserved. You run the risk of an OS macro messing up your code. Stop using an underscore as the first character of identifiers.

---

~Teste(){
    if (_Z != NULL)
            DELETE(_Z);
}

This is not needed. Asimple delete _Z would have been fine. _Z is going out of scope because it is in the destructor so no need to set it to NULL. The delete operator handles NULL pointers just fine.

~Test()
{    delete _Z;
}

---

Teste *Z(){
    _Z = new Teste;
    return _Z;
}

What happens if you call Z() multiple times (PS putting the * next to the Z rather than next to the Teste make it hard to read). Each time you call Z() the member variable _Z is given a new value. But what happens to the old value? Basically you are leaking it. Also by returning a pointer to an object owned inside Teste you are giving somebody else the opportunity to abuse the object (delete it etc). This is not good. There is no clear ownership indicated by this method.

Teste& Z()
{
    delete _Z;       // Destroy the old value
    _Z = new Teste;  // Allocate a new value.
    return *_Z;      // Return a reference. This indicates you are retaining ownership.
                     // Thus any user is not allowed to delete it.
                     // Also you should note in the docs that it is only valid
                     // until the next not const call on the object
}

---

void Z(Teste *value){
    value->AnyNum = 100;
    *_Z = *value;
}

You are copying the content of a newly constructed object (that contains a pointer) into another dynamically created object! What happens if _Z had not been allocated first. The constructor sets it to NULL so there is no guarantee that it has a valid value. Any object you allocate you should also delete. But here value is dynamically allocated passed into Z but never freed. The reason you get away with this is because the pointer is c opied into _Z and _Z is deleted when its destructor is destroyed.

---

Teste *b = new Teste, *a;

That's really heard to read. Don;t be lazy write it out properly. This is considered bad style and you would never get past any code review with that.

Teste* b = new Teste;
Teste* a; // Why not set it to NULL

---

a = b->Z();

Getting ab object for a. But who was destroying the object a or b?

b->Z(new Teste);

It just gets too convoluted after that.

Answer 5

(I tried to add this as a comment but that screws up the code..)

I'd aslo strongly suggest not to use

#ifndef DELETE
  #define DELETE(var) delete var, var = NULL
#endif

but instead something like

struct Deleter
{
  template< class tType >
  void operator() ( tType*& p )
  {
    delete p;
    p = 0;
  }
};

usage:

Deleter()( somePointerToDeleteAndSetToZero );

Answer 6

(not really an answer, but a comment wouldn't do)

The way you defined your macro is prone to a subtle errors (and the fact that no one spotted it so far just proves it). Consider your code:

if (_Z != NULL) // yes, this check is not needed, but that's not the point I'm trying to make
                DELETE(_Z);

What happens after the preprocessor pass:

if (_Z != 0)
        delete _Z; _Z = 0;

If you still have trouble seeing it, let me indent it properly:

if (_Z != 0)
        delete _Z;
_Z = 0;

It's not a big deal, given that particular if condition, but it will blow-up with anything else and you will spend ages trying to figure out why your pointers are suddenly NULL. That's why inline functions are preferred to macros - it's more difficult to mess them up.

---

Edit: ok, you used comma in your macro definition so you are safe... but I would still say it's safer to use [inline] function in this case. I'm not one of the do-not-use-macros-ever guys, but I wouldn't use them unless they are strictly necessary and they are not in this case

Answer 7

void Z(Teste *value){ value->AnyNum = 100; *_Z = *value; }

and

b->Z(new Teste);

creates a memory leak

the 'new Teste' never gets deleted, instead what you are doing is allocating a new object as parameter, then copying whatever is in there using *_Z = *value but the object is not deleted after the call.

if you were to write

Test* param - new Teste;
b->Z(param)
delete param;

it would be better

of course most would use boost::shared_ptr or something similar to avoid caring about delete and such