-2

Possible Duplicate:
Why does the use of ‘new’ cause memory leaks?

I am new in C++ memory profiling. Valgrind reported a leak in this line

m_propertyManager(new coral::PropertyManager);

So i modified it as

coral::PropertyManager Mgr;
m_propertyManager(&Mgr);

I guess &Mgr is deleted automatically but again valgrind is reporting leak in this.

Community
  • 1
  • 1
user1961044
  • 21
  • 1
  • 1
  • 2

2 Answers2

0

new coral::PropertyManager allocates a new PropertyManager on the heap, but because it is a temporary variable you will never release it. This is the standard Java idiom, since Java is a garbage-collected language where the GC will take care of releasing this dangling reference for you.

If you want to use new here for some reason, the proper way to do this would be as follows:

auto *pm = new coral::PropertyManager; // auto is C++11 syntax
m_propertyManager(pm);
delete pm; // when you're done using it

Your second option is correct, in that it allocates Mgr as an automatic variable on the stack, which will be released when the function exits. m_propertyManager(&Mgr); passes the address of Mgr to the function, which will allow it to modify the Mgr object (although this is probably better done by passing Mgr as a reference).

Just note that if m_propertyManager is an object that persists after the current scope exits, and if it stores the reference to Mgr somewhere, then when you exit the current scope and the Mgr object is destroyed you will find m_propertyManager holding a reference to invalid memory.

skoy
  • 266
  • 2
  • 10
  • better would be to use std::unique_ptr like m_propertyManager(std::unique_ptr(new coral::PropertyManager)); – mgr Jan 09 '13 at 12:12
  • @mgr: I may be mistaken, but isn't it so that you can't pass a copy of an unique pointer to a function? Wouldn't a std::shared_ptr be the wiser option in this use case, correct me when I'm wrong. – Skalli Jan 09 '13 at 12:56
  • yes you are right, a shared pointer is needed here. – mgr Jan 09 '13 at 21:40
0

If you allocate memory with new, you have to free it somewhere using delete. If m_propertyManager isn't supposed to manage the lifetime of PropertyManager, it will just throw the pointer away leaving the memory allocated and without access.

On the other hand, your second solution will crash. Look:

{
    coral::PropertyManager Mgr;
    m_propertyManager->SetManager(&Mgr); // You pass pointer to Mgr here
}
// Here Mgr no longer exists, so m_propertyManager
// now contains the pointer to non-existing object

You should either:

  • Create PropertyManager dynamically (using new), hold a pointer to it somewhere and free it explicitly at some point using delete;
  • Implement move ctor in PropertyManager and pass by value (such that m_propertyManager will implicitly automatically allocate its own instance of PropertyManager)
  • Use static allocation (as in your second example), but keep the instance in the place, where it will be kept alive at least as long as m_propertyManager.
  • Use some kind of automatic pointer (like std::shared_ptr or std::unique_ptr) inside m_propertyManager
Spook
  • 25,318
  • 18
  • 90
  • 167