Why am I getting a memory leak here?

Question

Main.cpp

#include<iostream>
#include "Queue.h"

using namespace std;
char* PotionTypeString(PotionType type)
{
    char* s = "UNKNOWN";

    switch (type) {
    case SPEED:
        s = "Speed";
        break;
    case STRENGTH:
        s = "Strength";
        break;
    case HEALTH:
        s = "Health";
        break;
    case WISDOM:
        s = "Wisdom";
        break;
    }
    return(s);
}

int main()
{
    Queue q;
    q.enqueue(WISDOM);
    q.dequeue();
    #ifdef _WIN32
    if (_CrtDumpMemoryLeaks()) {
        cout << "Memory leaks!" << endl;
    } else {
        cout << "No leaks" << endl;
    }
    #endif
    return 0;
}

Queue.cpp

#include "Queue.h"
#include <string.h>
#include <iostream>

using namespace std;

Queue::Queue() {
    front = NULL;
    rear = NULL;
    size = 0;
}

Queue::~Queue() {
    Node* cur = front;

    while (cur != NULL) {
        Node* temp = cur->next;
        delete cur;
        cur = temp;
    }
}

void Queue::enqueue(PotionType type) {
    Node* node = new Node();
    node->type = type;

    if (front == NULL) {
        front = node;
    }
    else {
        rear->next = node;
    }
    rear = node;
    size = size + 1;
}

PotionType Queue::dequeue() {
    PotionType toRet;
    if (front != NULL) {
        Node* node = new Node();

        node = front;
        front = front->next;

        toRet = node->type;
        delete(node);
        size = size - 1;
    }
    return toRet;
}

void Queue::print() {
    if (front == NULL) {
        cout << "Empty list" << endl;
    }
    else {
        Node * toPrint = new Node();
        toPrint = front;

        while (toPrint != NULL) {
            cout << PotionTypeString(toPrint->type) << endl;
            toPrint = toPrint->next;
        }
    }
}

In the main function I just instantiate an empty Queue, add a single item, then de-queue a single item, and I am getting memory leaks, I feel it has something to do with my dequeue method, or my destructor...

Though, I am kind of new to C++, so I am not entirely sure.

Anyone willing to help me out here?

Edit:

I put in the changes suggested by user4581301, and it fixed my memory leak issue when simply going

Queue q;
q.enqueue(WISDOM);
q.dequeue();

However, if I remove the q.dequeue() and leave it up to the destructor then, I receive a memory leak.

user4581301 · Accepted Answer · 2016-07-14T22:49:36.970

1

In Queue::dequeue

Node* node = new Node();

Allocates a new node who's address is promptly overwritten and leaked by

node = front;

Replacing both lines with

Node* node = front;

to immediately point node at front should be sufficient.

as Miles Budnek points out, the same error is in Queue::print. Do not new unless you absolutely have to, and all news must have a corresponding delete somewhere.

edited Jul 14 '16 at 22:49

answered Jul 14 '16 at 22:46

user4581301

33,082
7
33
54

The same thing happens in `Queue::print`. – Miles Budnek Jul 14 '16 at 22:47
@user4581301 I edited the OP with some more information. – Dylan Holmes Jul 14 '16 at 22:54
@DylanHolmes `Queue q;` allocates a static `Queue` that will not be destroyed until the `main` function exits. This is after your call to the leak checker. If you instead reduce the scope of `q` and then call the leak checker your leak will either be gone or you have a bug in the destructor. Try this:`{Queue q; q.enqueue(WISDOM);}` `q` will be destroyed at the closing brace and before the leak checker. I recommend reading [What is The Rule of Three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) because it will probably answer the next, as-yet unasked, question. – user4581301 Jul 15 '16 at 02:24

score 0 · Answer 2 · answered Jul 14 '16 at 22:56

After using a pointer, delete it and then set it to NULL. Example:

char* PotionTypeString(PotionType type)
{
    char* s = "UNKNOWN";

    switch (type) {
    }
return(s);
}

int main()
{
    Queue q;
    q.enqueue(WISDOM);
    q.dequeue();
    delete s;
    s = NULL;
    #ifdef _WIN32
    #endif
    return 0;
}

Of course, for this to work, s will have to be declared outside PotionTypeString() so that it won't go out of scope.

Why am I getting a memory leak here?

2 Answers2