Confused by vectors of classes: "The Big Three", deleting a class after a push_back, and resource management

Question

I have a class like this:

class cSerialMessage
{
    public:
        cSerialMessage(const enumMessages type, std::string txmessage = "") {rxAnswer="";};

        // Get/Set Methods
        // ...
        bool AddIntParam();     // Stores an int as parameter for the message
        void BuildCompleteMessage() // Build the whole message following the protocol rules

    private:
        enumMessages m_type;
        std::string m_txmessage, m_rxanswer;
        // [...]
};

Then i have another class which "manages" a queue of messages:

class cMsgManager
{
    public:
        cMsgManager();

        // [...]
        cSerialMessage* NewMessage(eMessages MSG);
        bool AddParameter(int Param);
        bool QueueMessage(bool FirstPosition);

    private:
        deque<cSerialMessage> cMS;
        cSerialMessage* tempMsg;
}

The relevant part is where the cSerialMessage are created and added to the deque.

cSerialMessage* cMsgManager::NewMessage (eMessages MSG)
{
    // Begin the creation of a new message


    if (tempMsg!=nullptr) {
        // there was already a temporary message
        OutputDebugString ("NOPE!");
        return nullptr;
    }
    tempMsg = new cSerialMessage(MSG);
    return tempMsg;
}
//------------------------------------------------------------------------------
bool cMsgManager::AddParameter (int Param)
{
    if (tempMsg==nullptr) {
        // this means that NewMessage() was'nt called before.
        OutputDebugString ("NOPE!);
        return false;
    }
    return tempMsg->AddIntParam(Param);
}
//------------------------------------------------------------------------------
bool cMsgManager::QueueMessage(bool FirstPosition)
{
    if (tempMsg==nullptr) {
        // this means that NewMessage() was'nt called before.
        OutputDebugString ("NOPE!);
        return false;
    }

    // Build the final message
    tempMsg->BuildCompleteMessage();

    if (FirstPosition) {
        cMS.push_front(*tempMsg);
    }else{
        cMS.push_back(*tempMsg);
    }

    delete tempMsg;

    tempMsg=nullptr;
    return true;
}

Despite all the questions about this topic ( this is very detailed), i'm still confused.

Should i delete my tempMsg? Is it copyed in the deque or, in the end, the data pointed by tempMsg are the one that will be later accessed from the deque? Or have i to create the copy-constructor and copy-assignement operator?

This code as is should not compile. `cMS.push_front(tempMsg);` should be an error as `tempMsg` is a `cSerialMessage*` while `cMS` holds `cSerialMessage`'s. Unless there is a reason `tempMsg` should be a `cSerialMessage`, not a `cSerialMessage*`. — NathanOliver, Nov 30 '18 at 14:00
`deque` holds copies of `cSerialMessage`s. I don't see why you are using pointers and `new` in the rest of the shown code at all. — Swordfish, Nov 30 '18 at 14:00
as a first approximation, if you remove all `*` in your code, it will be more correct ;) — 463035818_is_not_an_ai, Nov 30 '18 at 14:04
Seems to me like having a global tmp message is something bad in the design of this class. Should also probably be a deque of unique pointers. — Matthieu Brucher, Nov 30 '18 at 14:04
You should use smart pointer type for your message, no delete need as your messages will be released automatically. — tunglt, Nov 30 '18 at 14:05
I would advise you to make a much simpler example. THere is lots going on in your code that just distracts from your actual question. Maybe write a 4-5 lines example that just creates the object and pushes it into the vector — 463035818_is_not_an_ai, Nov 30 '18 at 14:06
You've got to address NathanOlivers point or the question is unanswerable. — john, Nov 30 '18 at 14:06
I don't see the point of `cMsgManager` over `using cMsgManager = std::deque`. You don't need `new` to create objects — Caleth, Nov 30 '18 at 15:15

ShadowRanger · Accepted Answer · 2018-12-21T12:03:39.803

Should i delete my tempMsg?

Yes, as written, you must either delete your tempMsg, or reuse the single instance for the life of the program (no making new ones after the first). Reusing it would likely require clearing it out before each reuse, which hardly seems worth it.

Is it copyed in the deque or, in the end, the data pointed by tempMsg are the one that will be later accessed from the deque?

It's copied; push_back receives references, but is documented only to copy or move (and since you passed an l-value, it's going to copy). You could save a little work by doing cMS.push_front(std::move(*tempMsg)); so as to empty out tempMsg (since you're just going to delete it after, so you may as well save the copies).

Or have i to create the copy-constructor and copy-assignement operator?

Assuming all the members of your cSerialMessage are themselves properly copyable (no raw pointers or the like), and you haven't defined any custom copy/move operations or destructors, you should be fine; the compiler generated copy constructor will work fine. cMsgManager on the other hand would need the full Rule of 3/5 constructors and destructors, because you didn't use a smart pointer or value semantics for tempMsg.

Note that this whole dynamic allocation thing is pointless/wasteful. You could just make tempMsg a cSerialMessage (not cSerialMessage*), and work with it by value. You could keep it an attribute of the instance, or just have NewMessage return the actual new message, not store a copy locally at all. The local copy is going to make threading or reentrant code a nightmare, so returning a new message by value, and having the caller manage it is probably a better idea.

Thanks for your nice explanation. – Parduz Dec 21 '18 at 10:23 — Parduz, Dec 21 '18 at 10:23

john · Answer 2 · 2018-11-30T14:19:54.670

Should I delete tempMsg? Yes you created it, someone has to delete it, and that should be you.

Is it copied into the deque? No, what is pointed to by tempMsg is copied into the deque, but tempMsg is still there afterwards and the object it points to is still there and so it needs deleting.

Do I have to create the copy-constructor and copy-assignment operator? Yes, or mark them as deleted if you are happy with cMsgManager being uncopyable.

Confused by vectors of classes: "The Big Three", deleting a class after a push_back, and resource management

2 Answers2