C++ --- how to write the correct code to release the allocated memory for this example?

Based on this http://sourcemaking.com/design_patterns/command/cpp/1, I think the posted code has memory leak b/c the allocated memory stored in the array is never released. I made one modification as follows,

#include "stdafx.h"
#include <iostream>
using namespace std;

class Giant
{
public:
    Giant()
    {
        m_id = s_next++;
    }
    void fee()
    {
        cout << m_id << "-fee  ";
    }
    void phi()
    {
        cout << m_id << "-phi  ";
    }
    void pheaux()
    {
        cout << m_id << "-pheaux  ";
    }
private:
    int m_id;
    static int s_next;
};
int Giant::s_next = 0;

// Define a Command interface with a method signature like execute().
class Command
{
public:
    typedef void(Giant:: *Action)();
    Command(Giant *object, Action method)
    {
        m_object = object;
        m_method = method;
    }
    void execute()
    {
        (m_object->*m_method)();
    }
private:
    G开发者_Go百科iant *m_object;
    Action m_method;
};

template <typename T> class Queue
{
public:
    Queue()
    {
        m_add = m_remove = 0;
    }
    void enque(T *c)
    {
        m_array[m_add] = c;
        m_add = (m_add + 1) % SIZE;
    }
    T *deque()
    {
        int temp = m_remove;
        m_remove = (m_remove + 1) % SIZE;
        return m_array[temp];
    }
private:
    enum
    {
        SIZE = 8
    };
    T *m_array[SIZE];
    int m_add, m_remove;
};

int main()
{
    Queue<Command> que;
    Command *input[] = 
    {
        new Command(new Giant, &Giant::fee), 
        new Command(new Giant, &Giant::phi),
        new Command(new Giant, &Giant::pheaux), 
        new Command(new Giant, &Giant::fee), 
        new Command(new Giant, &Giant::phi), 
        new Command(new Giant, &Giant::pheaux)
    };

    // 24 / 4 = 6
    size_t arrSize = sizeof(input)/sizeof(input[0]);

    for (int i = 0; i < 6; i++)
        que.enque(input[i]);

    for (int i = 0; i < 6; i++)
        que.deque()->execute();
    cout << '\n';

    // I add the following code to release the memory, however I am not sure
    // whether o not this is enough
    // There are two memory allocations in this code
    // 1-> new command and 2->new Giant
    // I think my code only release the memory allocated by new command.
    // Is this correct?
    for (int i = 0; i < 6; i++)
    {
        delete input[i];
        input[i] = NULL;
    }
}

Please my question above.

Thank you

I think my code only release the memory allocated by new command. Is this correct?

Yes it is correct. But...

Why would you even bother allocating those on the heap? Just create them on the stack, and pass pointers in where needed. Then you will have zero chance of forgetting to deallocate them.

Giant giant;
std::vector<Command> input;
input.push_back(Command(&giant, &Giant::fee));
input.push_back(Command(&giant, &Giant::phi));
input.push_back(Command(&giant, &Giant::pheaux));
input.push_back(Command(&giant, &Giant::fee));
input.push_back(Command(&giant, &Giant::phi));
input.push_back(Command(&giant, &Giant::pheaux));

// ...

for (int i = 0; i < input.size(); i++)
    que.enque(&input[i]);

You should look up the RAII idiom. It's a bad practice in C++ to put a new anywhere outside a constructor (unless you pass it straight to a smart pointer), and it's bad practice to put a delete anywhere outside a destructor. It's also bad to try to allocate more than one thing at a time, without wrapping each one separately in RAII or a smart pointer.

You can make code that will work correctly that doesn't use RAII/smart pointers. But it will be extremely difficult to do while guaranteeing no memory leaks under any circumstances, especially if an exception is thrown anywhere in your code.

Also, you can use an existing queue data structure rather than rolling your own. I highly recommend that you avoid rolling your own data structures unless you really have to, or you're specifically trying to learn how a data structure works internally (and you're not putting that code into production). This code seems to be a demo of member function pointers, so I think the queue implementation is incidental complexity.

Use std::queue instead.

You are correct - delete input[i] will only delete the instances of Command, not the instances of Giant.

"I think my code only release the memory allocated by new command. Is this correct?"

Yes, if you mean what I think you mean. The Command instances are deallocated. But each Command instance holds a pointer to a dynamically allocated Giant, which is not deallocated.

A simple cure for this particular program is to not use dynamic allocation.

More generally, use container classes like std::queue and smart pointers like std::auto_ptr as appropriate.

Cheers & hth.,

Yes, you will leak all those giants. For a small test, I'd just put them on the stack rather than having the herd of wildebeest approach:.

    Queue<Command> que;
    Giant giant;
    Command input[] = 
    {
        Command(&giant, &Giant::fee), 
        Command(&giant, &Giant::phi),
        Command(&giant, &Giant::pheaux), 
        Command(&giant, &Giant::fee), 
        Command(&giant, &Giant::phi), 
        Command(&giant, &Giant::pheaux)
    };

    // 24 / 4 = 6
    size_t arrSize = sizeof(input)/sizeof(input[0]);

    for (int i = 0; i < arrSize; i++)
        que.enque(input+i);

You are correct. There is a leak.

You need to use smart pointers to indicate ownership:

Personally I would use the Queue object as the owner.
But there are other options (and your solution may very on context that you have not elucidated).

For example the object input could take ownership. But you would need to turn this from a simple array into something that understood ownership and thus had constructor/destructor.

The following steps are me thinking through the Queue object taking ownership. But the same principles will apply to any object that takes ownership.

Step 1:

Make sure Queue takes ownership and tidies up

template <typename T> class Queue
{
    // This object takes ownership so make sure the it tides itself up:

    // Options 1: Use an array of smart pointer:
    std::auto_ptr<T> m_array[SIZE];  // Note using auto_ptr will make the object uncopyable.

    // Options 2: Use a smarter pointer
    boost::shared_ptr<T> m_array[SIZE];

    // Option 3: Use a smart container
    boost::ptr_vector<T> m_array; // Initialize in the destructor to SIZE

    // Option 4: Manage the array yourself.
    private: Queue(Queue const& copy);            // don't define
    private: Queue& operator=(Queue const& copy); // don't define
    public: ~Queue()  { /* Loop over m_array and call delete on each member */ }
};

Step 2:

Sicnce you manage the pointers inside the queue. You should make the interface that accepts pointers explicit.

void enque(std::auto_ptr<T> c) // This explicitly tells the caller that you are
                               // taking ownership and they should not try and manage
                               // object after you receive it.

Step 3:
Decide what deque actually does.
Does it release ownership? In which case you should return a smart pointer.
Does it give shared ownership? In which case you will need a shared pointer.
Does it give out an object for simple usge in an application? Return a reference.