4
\$\begingroup\$

I was going through a code snippet...

try
{
    ...
    // Memory is allocated here...
    NmsMessage* pNmsMessage = new NmsMessage(NMS_MODEM_DIAL_NUMBER);
    ...
    {
        // pNmsMessage is passed to another thread.
        pThis->m_pDeviceThread->PostThreadMessage(WM_NMS_MESSAGE, 0,pNmsMessage); 
    }
    return NOERROR;
}
catch(...) 
{
    ... 
}

In the PostThreadMessage, memory pointed by pNmsMessage is getting released.

My question:

  1. Is this the right way to handle memory in case of threaded environment?
  2. If it is not threaded environment, is it better to release the memory before returning from the function and also in the catch block(I don't want to use smart pointers)?
\$\endgroup\$
  • 1
    \$\begingroup\$ Why don't you want to use smart pointers? You may as well be using C at that point. \$\endgroup\$ – Martin York Jun 6 '14 at 10:22
2
\$\begingroup\$

I could see passing the pointer to another thread, and the other thread deleting the object. (Gives me the willies, but it at least makes sense.) But if PostThreadMessage itself is doing the deletion, that feels wrong. I'm used to having a delete for every new, and PostThreadMessage is currently breaking that all to hell. Now i have to go rooting around just to figure out who owns an object, so i don't accidentally delete it twice and open up a wormhole or something.

If the pointee doesn't need to outlive the call to PostThreadMessage, then you should be able to put it in a local variable and pass a pointer to the local. Of course, that also means PostThreadMessage needs to keep its paws off the delete button. :P

try {
    ...
    NmsMessage nmsMessage(NMS_MODEM_DIAL_NUMBER);
    ...
    {
        pThis->m_pDeviceThread->PostThreadMessage(WM_NMS_MESSAGE, 0, &nmsMessage);
    }
    return NOERROR;
}
catch(...) {
    ... 
}

nmsMessage, having "automatic storage duration", will be deleted when control leaves the block. Ownership and intent are clearer, you don't need to fart around with new and delete, and C++ cleans up for you automatically whether you return normally or throw an exception.

If PostThreadMessage wants the message info to outlive this code, seems to me that it should be making its own copy. But it kinda sounds like it's already doing that.

(Note, most of this answer assumes that you can change PostThreadMessage. If you can't -- for example, if it's part of some API you're stuck using -- then the only guarantees you have are the ones the API makes. If it doesn't promise that pNmsMessage is either deleted or not in each case that may occur, then you have a pretty serious problem; any dynamically allocated object must be deleted, but deleting it twice can cause major problems.)

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.