How to avoid this memory leak?

Question

This is my code:

void MIDITest::CreateNoteBlock() {
    IMidiMsgExt* midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(57, 100, 0, 0, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(60, 100, 0, tickSize * 38, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(62, 100, 0, 0, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(65, 100, 0, tickSize * 32, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(57, 0, tickSize * 111, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(60, 0, tickSize * 111, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(62, 0, tickSize * 75, 0);
    queuedNotes.insert(*midiMessage);

    midiMessage = new IMidiMsgExt;
    midiMessage->MakemidiMessageMsg(65, 0, tickSize * 105, 0);
    queuedNotes.insert(*midiMessage);
}   

so at every new operator it will allocate a block of memory.

Should I use free after any insert inside the queuedNotes? Or it will be released after void function return? (i.e. the brackets of CreateNoteBlock).

Or I can "reuse" each time the midiMessage pointer for a new IMidiMsgExt?

Answer 1

Try to make your API more C++ like.

Use one object on the stack instead of creating a lot of new ones in the heap.

void MIDITest::CreateNoteBlock() {
    IMidiMsgExt midiMessage;

    midiMessage.MakemidiMessageMsg(57, 100, 0, 0, 0);
    queuedNotes.insert(midiMessage);

    midiMessage.MakemidiMessageMsg(60, 100, 0, tickSize * 38, 0);
    queuedNotes.insert(midiMessage);

    // ...
}

Initialize your objects in the constructor. Define an operator = (const IMidiMsgExt&) for IMidiMsgExt.

void MIDITest::CreateNoteBlock() {
    IMidiMsgExt midiMessage(57, 100, 0, 0, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt(60, 100, 0, tickSize * 38, 0);
    queuedNotes.insert(midiMessage);

    // ...
}

I guess, that insert() expects a const IMidiMsgExt&. So you can directly pass freshly initialized objects:

void MIDITest::CreateNoteBlock() {
    queuedNotes.insert({57, 100, 0, 0, 0});
    queuedNotes.insert({60, 100, 0, tickSize * 38, 0});

    // ...
}

BTW: you should prefer to use e.g. std::queue<> for queuedNotes. Then you'll not use insert(), but push(), or emplace(). The advantage of emplace() is, that it constructs the object in the container instead of first creating it and then copying it to the container:

void MIDITest::CreateNoteBlock() {
    queuedNotes.emplace(57, 100, 0, 0, 0);
    queuedNotes.emplace(60, 100, 0, tickSize * 38, 0);

    // ...
}

Also your typename IMidiMsgExt signals to me, that you are trying to mimic the C# thinking in C++. It is possible, but usually it is not the preferred solution. From your question I don't know enough about your class tree and the underlying requirements to provide a suggestion, but in C++ that usually is a code smell.

Answer 2

The answer is not to use new at all. Create a object with automatic storage duration

IMidiMsgExt midiMessage;

And then you can keep calling MakemidiMessageMsg and insert a copy of the message into the multiset.

midiMessage.MakemidiMessageMsg(57, 100, 0, 0, 0);
queuedNotes.insert(midiMessage);
midiMessage.MakemidiMessageMsg(60, 100, 0, tickSize * 38, 0);
queuedNotes.insert(midiMessage);
//...

Now the multiset has a copy of all of the messages and at the end of the function midiMessage is destroyed and no memory management needs to be done.

If IMidiMsgExt has a constructor that is like MakemidiMessageMsg where you can construct a complete message then you could boild this down even further and use something like

queuedNotes.insert(IMidiMsgExt(57, 100, 0, 0, 0));
queuedNotes.insert(IMidiMsgExt(60, 100, 0, tickSize * 38, 0));

And now we don't even need midiMessage.

Answer 3

It seem redundant to dynamically allocate IMidiMsgExt using new. You can just allocate it directly on the stack (no pointers), and then it will be destroyed when your method returns. I.e.:

void MIDITest::CreateNoteBlock() {
    IMidiMsgExt midiMessage();
    midiMessage.MakemidiMessageMsg(57, 100, 0, 0, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(60, 100, 0, tickSize * 38, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(62, 100, 0, 0, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(65, 100, 0, tickSize * 32, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(57, 0, tickSize * 111, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(60, 0, tickSize * 111, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(62, 0, tickSize * 75, 0);
    queuedNotes.insert(midiMessage);

    midiMessage = IMidiMsgExt();
    midiMessage.MakemidiMessageMsg(65, 0, tickSize * 105, 0);
    queuedNotes.insert(midiMessage);
}

Answer 4

Do you come from a Java or C# background? In C++ you don't have to use new to create objects, just declaring them will work fine:

IMidiMsgExt midiMessage;
midiMessage.MakemidiMessageMsg(57, 100, 0, 0, 0);
queuedNotes.insert(midiMessage);

It's either that (which is my recommended solution), or you have to explicitly free the objects:

IMidiMsgExt* midiMessage = new IMidiMsgExt;
midiMessage->MakemidiMessageMsg(57, 100, 0, 0, 0);
queuedNotes.insert(*midiMessage);
delete miniMessage;