2

I profiled my code recently with Visual Leak Detector for the first time, and it indicated a leak in a vector, which I wasn't expecting. The code is like so:

void func()
{
    std::vector<MsgUnit> msgVec;

    do
    {
        // msgVec.clear(); // do I need to do this to avoid a leak?
        msgVec = m_obj->returnMsgUnitVector();
    }
    while (someConditionNotMet);

    // process msgVec

    return;
}

MsgUnit has a copy constructor and destructor.

I haven't found the time to do in-depth testing, but a quick fix seems to indicate that uncommenting the clear() method removes the leak.

I'm wondering what the standard says about this behaviour. Do I need to clear the vector before assigning to it, to avoid a leak?

aerobot
  • 195
  • 1
  • 8

3 Answers3

10

No, assignment will make the destination vector value-equivalent to the source vector. It will internally do what it needs to ensure this without leaking. [Assuming that the copy-constructor, assignment and destructors of your type don't leak]

David Rodríguez - dribeas
  • 204,818
  • 23
  • 294
  • 489
  • And the fact that he's seeing a difference with `clear` suggests that there may be a problem with his assignment operator (although we can't really be sure). – James Kanze Dec 09 '13 at 14:36
  • 1
    Ah, you reminded me of the assignment operator for the type. I wasn't thinking, and confused it with the copy constructor. Went to look at the library's code, and indeed the assignment operator forgot to delete its old resource. – aerobot Dec 09 '13 at 14:43
3

MsgUnit has a copy constructor and destructor.

I guess it doesn't have an copy-assignment operator (per the Rule of Three), which is why you get leaks or worse when you reassign them. The implicitly-generated operator will simply assign each class member. If some of those members are dumb pointers to manually-managed resources, then you'll lose those pointers and leak the resources.

Either implement that or, better still, redesign MsgUnit to use a smart pointer (or similar) to manage its dynamic resources automatically with no need for you to mess around with destructors and the like.

Community
  • 1
  • 1
Mike Seymour
  • 249,747
  • 28
  • 448
  • 644
  • Why would that result in a leak, rather than a compilation error? – Lightness Races in Orbit Dec 09 '13 at 14:06
  • it would not be an error, the default assignment semantics would be implemented by the compiler which in many cases are not what the programmer wants. The vector does not leak, but a default assignment might cause the dynamic data within MsgUnit to leak. – DNT Dec 09 '13 at 14:08
  • @LightnessRacesinOrbit: Why would there be a compilation error? It would use the implicit copy-assignment operator, which will (almost certainly) do the wrong thing with the pointers to managed resources. – Mike Seymour Dec 09 '13 at 14:11
0

MsgUnit should also have a operator=(const MsgUnit &) defined to be fully compliant with STL.

DNT
  • 2,356
  • 14
  • 16
  • Or, with C++11, a move-assignment operator (`operator=(MsgUnit&&)`). – Pete Becker Dec 09 '13 at 14:37
  • @DNT do you have an official reference for *compliance with STL* at hand? – Wolf Dec 09 '13 at 15:47
  • @Wolf: I don't see what the STL has to do with it. – Lightness Races in Orbit Dec 09 '13 at 16:07
  • @Wolf Linguistic semantics, shortcuts, and implications... In this context and only in this context, 'fully compliant' refers to the 'Rule of Three' which in itself is not 'official' in the sense of 'follow or crash', but is generally a widely accepted good practice when working with STL containers. – DNT Dec 09 '13 at 16:25