views:

212

answers:

8
#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?

+4  A: 

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);
}
AraK
a copy constructor have to be build manually? eg: deepcopy(a, b){ a->m = b->m; } etc...
Jonathan
If your class has a pointer to anything outside itself, and it isn't just something supplied for its use (like a file pointer for a file-writing class), it needs to make sure it's copied properly and deleted properly. It normally needs a manual destructor, copy constructor, and assignment operator.
David Thornley
The Z function isn't checking for a null _z pointer. You have a constructor that can leave _z null, so this (with a different main) could crash. "b->Z(new Teste);" should probably be "b->Z (Teste ());" (use a temporary, make sure it destructs). While AraK is correct about the destructor NULL check, you might keep it anyway - it warns readers that the pointer might be null. Oh, and you should learn both smart pointers *and* dumb pointers, so IMO you get credits for learning this stuff despite the smart pointer comments.
Steve314
+2  A: 

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.

David Thornley
cf. http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier/228797#228797
James McNellis
yeah, that was just an example
Jonathan
I don't understand this rules... For example, if I have a member called _Hmm, it won't be affected by the OS or anything like that, right? the directives are __STUFF__ etc... If they don't affect it, using underscore or not, its up to the likes of the developer? Just a question! xD
Jonathan
Read that post I linked; it explains all of the rules regarding underscores. If you want to be 100% safe without worrying about the rules, don't use underscores in your identifiers.
James McNellis
If you had an identifier called __func__ and then moved to C99, your code would fail, as they added that identifier to those an implementation is required to provide. The same could apply to any other identifier starting in _. If your variable is _THREAD and you move to an OS decides to make _THREAD a macro, then your code will break.
Pete Kirkham
should be `__func__`
Pete Kirkham
@Jonathan: Take a look at some of the template code in your library. It needs to use variables, and they have to have names distinct from the user variables. If some vector<> code used _Z, and you write something like foo.push_back(_Z), the compiler wouldn't know which _Z was which. That's not the only potential problem, but remember that the implementation needs to use some variables, if you name a variable the same thing you've got problems, and if you avoid the reserved name classes you're safe from that.
David Thornley
I thought all leading underscores, irrespective of next letter (plus, as you say, all double-underscores) were reserved? Could be wrong, I guess. In any case, I'll recommend the convention with a "p" or "p_" prefix for paramaters, "l" or "l_" for local vars, and "m" or "m_" for member vars (no prefix for methods etc). Saves on inventing multiple names for the same thing in different places. Also, a single *trailing* underscore is legal as a last resort (I tend to use it for include guards).
Steve314
@Steve314: IIRC, it's identifiers starting with leading underscores in the global namespace, identifiers starting with an underscore followed by an uppercase letter, and identifiers with two consecutive underscores anywhere.
sbi
+1  A: 

(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 );
stijn
What do you need a class for this? What's wrong with a simple function? `template<class T> void do_delete(T*p=0;}`
sbi
whats the problem with a simple macro?
Jonathan
@sbi the functionality is indeed the same, I used the struct because it can be passed to std::for_each and the likes.@Jonathan you should avoid macros for anything that can be as well be expressed with the actual language. Here's a perfect example of what the problem is with a simple macro: http://www.parashift.com/c++-faq-lite/inline-functions.html#faq-9.5
stijn
@Jonathan: Have you tried your macro this way: `DELETE(p+idx)`?
sbi
@stijn: You certainly have a very valid point there. I'd provide both, though, since the `Deleter()(p)` syntax is just to awkward.
sbi
A: 

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

Anders K.
new Teste gets delete on the destructor of the class, but the older _Z never gets deleted, right? boost? quick google told me is a very known library. Maybe I will take a look later, ty
Jonathan
yes that is correct. boost will also help you a lot.
Anders K.
+5  A: 

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;
}
Todd Gardner
thanks, that clarified my question. that purpose of shared_ptr is to not have to delete stuff in the destructor?
Jonathan
The purpose of shared_ptr<> and auto_ptr<> and other smart pointers is to not have to explicitly delete stuff, but have it deleted automatically. Smart pointers aren't perfect, but they eliminate entire classes of tough problems.
David Thornley
A shared_ptr protects resources using a RAII pattern ( http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization). A nice side effect is you don't have to write any explicit deletes, and it clarifies ownership semantics, i.e. does Teste "own" (is responsible for deleting) the pointers it is passed? Unclear in the original, but in the rewritten code, it's clear Teste doesn't "own" but "shares" them.
Todd Gardner
+2  A: 

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.

Martin York
thanks for the post, it was very helpful indeed. This code I wrote isnt my project, I was just trying to understand some conceptions over references and ptr. You said that when you "own" teh variable, then the getter should return a reference instead of the pointer right? Ill check about it!That solves my question. I wasn't good on my question... what I wanted to know, is how to make sure that the *a pointer is still ok to use after using the setter method, understand? My english fail sometimes words just don't come... Thanks a lot for the help, you and everyone. This site is just that cool.
Jonathan
Hi, the assignment operator is like the copy constructor? ie: Should assign by deleting and copying from A to B?
Jonathan
Basically yes. But you can just copy the value of a pointer. You must copy the content.
Martin York
+1  A: 

(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

sbk
A: 

this is weird.

seeing your responses, then I went to google to check out what they say about building public libraries... syntaxes, operators, pointers etc etc

and they say one would never overload operator except the << operator but the rule that I was going to use(the ule I think) that Martin York told me says that I should overload the = operator...

and in fact, there are more stuff there that don't agree with stuff I've seen here

what now? -_-"

Jonathan