Question

I initialise the SmartPtr class with new Time(1,0,0).

    //main.cpp 
    int main()
{
    SmartPtr pTime0(new Time(0,0,1));
}

Nowhere am I calling delete on new Time(1,0,0). Everything works fine, the program complies and runs. but I am confused -- where should/should not I delete Time(1,0,0) ?

I do not understand the concept of temporary objects creating and deleting here. I know whenever I write new somewhere I MUST WRITE delete ! Could someone please explain where delete Time(1,0,0) takes place?

SmartPtr pTime0(new Time(0,0,1)) <-- new here returns a pointer to a newly allocated memory, and then in ctor I allocate new memory the second time??

//SmartPtr.cpp 

SmartPtr::SmartPtr(Pointee * p):_pointee(new Pointee(*p))
{}

SmartPtr::~SmartPtr()
{
    delete _pointee; 
}
Était-ce utile?

La solution

I don't know the details of your SmartPtr class.

In any case, if you have a constructor like this:

SmartPtr::SmartPtr(Pointee * p):_pointee(new Pointee(*p))
{}

and this is the destructor:

SmartPtr::~SmartPtr()
{
    delete _pointee; 
}

then with this code:

SmartPtr pTime0(new Time(0,0,1));

you leak one instance of Time(0,0,1).

In fact, you have one more new than delete (2 news and 1 delete):

Step #1: You call new Time(0,0,1) and create a new object on the heap.
(new count == 1)

Step #2: You pass this pointer to SmartPtr constructor, which deep copies previously created object and allocates a new copy on the heap, and keeps track of this copy via its _pointee data member.
(new count == 2)

Step #3: When the SmartPtr destructor runs, it deletes the instance pointed by _pointee data member, but you leaked the firts Time(...) created on the heap with new Time(0,0,1).
(delete count == 1; new count == 2)

A possible fix for that could be to just have this constructor:

SmartPtr::SmartPtr(Pointee * p)
    : _pointee(p) // <--- transfer ownerhsip (no deep copies) !
{}

An easy way to identify potential leaks in these cases is to put some console tracing output in Time class constructors and destructor, and check that the trace output of destructor matches the ones of constructors, e.g.:

Time::Time(....)
{
    // Do construction work....

    std::cout << "Time constructor\n";
}

Time::~Time(....)
{
    // Do destructor work....

    std::cout << "Time destructor\n";
}

The total count of "Time constructor" strings should match the total count of "Time destructor" strings.

Autres conseils

Two ways to fix:

Method A, caller allocates, SmartPtr takes ownership:

SmartPtr::SmartPtr(Pointee * p):_pointee(p)
{
}

Method B, caller provides content and SmartPtr allocates:

SmartPtr::SmartPtr(Pointee v):_pointee(new Pointee(std::move(v)))
{
}

And the destructor remains the same:

SmartPtr::~SmartPtr()
{
    delete _pointee; 
}

The expression new Time(0,0,1) create a temporary pointer to a permanent object on the heap. The temporary pointer will indeed be destroyed automatically (which is a no-op), leaving the object still on the heap but unreferenced. A leak has occurred.

To prevent the leak, make sure to store the pointer somewhere and ensure that delete is eventually called on it.

You can write your applications according to the principle never type new.
Combine this with existing smart-pointers, and it becomes:

#include <memory>    // this is where the smart-pointers live
#include "Time.h"    // or whatever header defines your "Time" class

int main()
{
    // note that make_shared is essentially a forwarding constructor,
    // give it whatever parameters Time's constructor would take
    auto p = std::make_shared<Time>(0,0,1); 
    // use p here
}

And there will be no leak of anything ever.

"Never type new" should apply to all application programming, with the only exception if you have to write a low level resource management library.

Note that if a class does resource management, it should be it's sole function.

All your other classes should follow rule of zero

Licencié sous: CC-BY-SA avec attribution
Non affilié à StackOverflow
scroll top