CODEWITHSUNDEEP
c++memory-leaksstack
user1740222
user1740222

Reputation: 41

Memory leak in stacks/queues c++

I have an assignment to write a Stacks class using a list and a Queue class using two Stacks. I've completed the assignment, but running the valgrind I find that I have a memory leak in the following code:

T Stack<T>::pop()
{
    T *n = new T;
    *n = myStack.front();
    myStack.pop_front();
    return *n;
}

I can't delete the pointer after I return it so I'm not sure how to fix it. Thanks in advance.

Upvotes: 3

Views: 1236

Answers (7)

Sebastian Mach
Sebastian Mach

Reputation: 39089

Copying semantics is one of the biggest strengths of C++, because you can put the compiler and the author of type ´T´ to blame:

T Stack<T>::pop() // may throw
{
    T ret = myStack.front();
    myStack.pop_front();
    return ret;
}

Note though that this is a sub-ideal form of a pop-function. Upon copying, an exception might be thrown, which makes implementing an exception safe pop-function essentially impossible.

The container std::stack<> solves by making the return type void:

void Stack<T>::pop() // no-throw [if T is written by a sane coder]
{        
    myStack.pop_front();
}

T Stack<T>::back() // may throw
{        
    return myStack.front();
}

This provides you with a mean to clean up your stack upon destruction without throwing exceptions (throwing destructors in C++ is, by convention (not by the standard), forbidden).

Upvotes: 0

rbhawsar
rbhawsar

Reputation: 813

T *n = new T; you are creating T using new and not using it. that is the problem.

Upvotes: 0

Digikata
Digikata

Reputation: 1959

You've got multiple answers giving the correct code, but the reason your existing code was wrong goes something like this:

T Stack<T>::pop()
{
    T *n = new T;          // allocates dynamic memory
    *n = myStack.front();  // reference to T from front() copied to allocated T object
    myStack.pop_front();   // removes the T in the stack
    return *n;  // copies your allocated T object to the return value
    // your pointer variable goes out of scope, that address is stored nowhere,
    // this is where the leak occurs...
}

Upvotes: 0

ISTB
ISTB

Reputation: 1817

On your place, I would stop using raw pointers and change to shared_ptr. Is is much safer.

Upvotes: 0

Lyubomir Vasilev
Lyubomir Vasilev

Reputation: 3030

Why do you even need to use new? You can make a copy of the stack's top value like this:

T Stack<T>::pop()
{
    T n = myStack.front();
    myStack.pop_front();
    return n;
}

So there are no allocations and no leaks;

Upvotes: 4

Ram
Ram

Reputation: 3133

Make a copy and then clear the memory if any inside pop_front. 

    T Stack<T>::pop()
    {
        T ret = myStack.front();
        myStack.pop_front();        
        return ret;
    }

Upvotes: 3

perilbrain
perilbrain

Reputation: 8197

You should have rather used

T n = myStack.front();

Upvotes: 0

Related Questions