-1

I ran purify on my code which runs in Solaris and it shows lot of memory leaks. But I checked the code and most of the leaks seem to be invalid.

For eg,

File1.cpp

Obj* getMyObj()
{
    Obj* obj = NULL;
    if(condition)
    {
        obj = new Obj();   //Purify is reporting leak here
        //Fill obj
    }

    ...
    return obj;
}

File2.cpp

void myfunc()
{
    Obj* myobj = getMyObj();

   if(myobj == NULL)
       return;
    ...
    ...

    delete myobj;    //The object is deleted here
}

Even though the object is destroyed properly in File2.cpp, why is purify reporting leak in File1.cpp?

EDIT

The NULL check was just a typo, I corrected it.

Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
cppcoder
  • 22,227
  • 6
  • 56
  • 81
  • Ar you showing all your code? Is this a factory function where you build different objects depending on the "condition"? In that case, do class Obj have a virtual destructor? – Bakery Jul 31 '12 at 05:20
  • @NiklasKarlsson - This is not a factory function. But, the class Obj has a virtual destructor. – cppcoder Jul 31 '12 at 05:30

3 Answers3

2

Even though the object is destroyed properly in File2.cpp, [...]

This assumption is wrong.

Obj* myobj = getMyObj();

If getMyObj actually creates an object, it won't return a null pointer. That means the condition in the following if is true, and then the function returns immediately.

if(myobj)
    return;

No further code executes in that function, so it's never destroyed.

I recommend the use of smart pointers instead of this kind of manual management, as this kind of errors just goes away. With C++11 you can use std::unique_ptr, otherwise you can use std::auto_ptr if you're careful.

R. Martinho Fernandes
  • 228,013
  • 71
  • 433
  • 510
  • @DeadMG - What does one do, if he does not have access to `boost` or `C++11`? – cppcoder Jul 31 '12 at 05:09
  • @cppcoder Write your own smart pointer class. Something like boost's shared_ptr is straightforward enough, even though you're unlikely to do it quite as well as the boost devs did. – jahhaj Jul 31 '12 at 05:28
  • @jahhaj `shared_ptr` is far from straightforward, unless you mean it's straightforward to write a buggy one. `auto_ptr` is fine if you're careful and don't drop it on containers and the like. – R. Martinho Fernandes Jul 31 '12 at 06:01
  • @R.Martinho Fernandes Hmm, what kind of bugs are you thinking of? Reference counted smart pointers is a topic well covered in books etc. – jahhaj Jul 31 '12 at 06:05
  • @jahhaj: Feel free to try and drop it in for code review http://codereview.stackexchange.com/questions were we will be sure to point out the problems. Smart pointers are some of the hardest things to get correct and the boost guys spent years getting it working correctly. – Martin York Jul 31 '12 at 07:54
  • @LokiAstari Hmm, I think I'll pass and just use boost. But the OP is not able to do this, I was just pointing out that it should be possible for him to write a smart pointer class that is adequate for his needs. That is certainly a better route than attempting to use raw pointers. – jahhaj Jul 31 '12 at 08:08
  • 1
    @jahhaj: std::auto_ptr would be a far better choice than writing your own. – Martin York Jul 31 '12 at 13:53
  • @LokiAstari But only if std:auto_ptr is adequate for his needs. I find it very limited and therefore I've never used it much. – jahhaj Jul 31 '12 at 18:26
  • @jahhaj: Its exactly std::unique_ptr (apart from you don't use std::move). Even if it was not adequate its still better than anything we could write. – Martin York Jul 31 '12 at 21:15
  • It's exactly `unique_ptr`, except it sucks horrifically. There's a reason `unique_ptr` was created and it's because `auto_ptr` has serious limitations and several nasty surprises. – Puppy Aug 01 '12 at 00:16
0
if(myobj)
   return;

After object created return proceeded and delete never performed

You need modify code:

-if(myobj)
+if(myobj==NULL)
CyberDem0n
  • 14,545
  • 1
  • 34
  • 24
0

In file1.cpp

Obj* getMyObj();

the function is kind of unsafe since the caller of the function needs to know that he must delete the object returned but it is not clear from the function that it is necessary

better is to use a smart pointer like a shared_ptr instead of the raw pointer, it would then be clear that the object returned is allocated on the heap and also how it is destroyed (if allocated).

std::shared_ptr<Obj> getMyObj();
AndersK
  • 35,813
  • 6
  • 60
  • 86