0

My templated queue's dequeue function works fine for a queue of string, but if I use my custom Robot class, it crashes upon trying to delete the pointer. I'm curious as to why.

For example, in main.cpp

#include <iostream>
#include <cstdlib>
#include <cstring>
#include "robotqueue.h"
#include "robotcustomer.h"
#include "servicestation.h"

using namespace std;

int main()
{
    //- TEST ONE: QUEUE<STRING> -//
    RobotQueue < string > stringQueue;
    string a("Tim");
    string b("Greg");

    stringQueue.enqueue(a);
    stringQueue.enqueue(b);
    stringQueue.dequeue();
    stringQueue.dequeue();

    //- TEST TWO: QUEUE<RobotCustomer> -//
    RobotQueue < RobotCustomer > robotQueue;
    RobotCustomer e("Tim",3);
    RobotCustomer f("Greg",5);

    robotQueue.enqueue(e);
    robotQueue.enqueue(f);
    robotQueue.dequeue();            <--- Segfault occurs here
    robotQueue.dequeue();
    return 0;
}

the string queue works fine, but I get this error:

***Error in `q': munmap_chunk(): invalid pointer: 0x0000000001d6c108 ***
Aborted (core dumped)

My templated queue looks about like this (dunno if this ya need more).

robotqueue.hpp

// Default Constructor
template <typename T>
RobotQueue<T>::RobotQueue()
{
    m_size = 0;
    m_front = NULL;
    m_back = NULL;
}
// Default destructor
template <typename T>
RobotQueue<T>::~RobotQueue() 
{
    Node<T>* currNode = m_front, *nextNode = NULL;
    while ( currNode != NULL )
    {
        nextNode = currNode->m_next;
        delete currNode;
        currNode = nextNode;
    }
    m_size = 0;
}

template <typename T>
void RobotQueue<T>::enqueue(const T& x)
{
    Node<T> *newNode = new Node<T>;
    newNode->m_data = x;
    newNode->m_next = NULL;
    if(m_front == NULL)
        m_front = newNode;
    else
        m_back->m_next = newNode;
    m_back = newNode;
    m_size++;                           // Increments queue size
    return;
}

template <typename T>
void RobotQueue<T>::dequeue()
{
    Node<T>* tempNode = new Node<T>;
    if(m_front == NULL)
        cout << "dequeue error: Queue is empty" << endl;
    else
    {
        tempNode = m_front;
        m_front = m_front->m_next;
        delete tempNode;     <-- Segfault occurs here in RobotCustomer class
        m_size--;                       // Increments queue size
    }
    return;
}

I'm assuming it has to do with RobotCustomer being a class so m_data can't point to it or something? Not an expert here :p

RobotCustomer.h

/* ------------------  Class RobotCustomer ------------------ */
class RobotCustomer
{
private:
  string m_name;                // Name of Robot
  string* m_reqServices;        // Array of services requeseted
  int m_numServices;            // Number of services requested
  int m_currService;            // Number of services compelted
  bool m_busy;                  // Logic for if robot is in line/servicing
  bool m_done;                  // Logic for if robot is done
public:
  //- functions and such that I don't think affect the queue -//

Thanks for your time :)

---------------------UPDATED_WITH CONSTRUCTORS/DECONSTRUCTORS-------------------

RobotCustomer.cpp

// Default Constructor
RobotCustomer::RobotCustomer()
{
    m_name = "";
    m_numServices = 0;
    m_currService = 0;
    m_busy = false;
    m_done = false;
}
// Overloaded Constructor
RobotCustomer::RobotCustomer(string n, int x)
{
   m_name = n;
   m_numServices = x;
   m_reqServices = new string[m_numServices]; 
   m_currService = 0;
   m_busy = false;
   m_done = false;
}

// Default Destructor
RobotCustomer::~RobotCustomer()
{
    delete m_reqServices;
}
Wezabi
  • 3
  • 3
  • If you want an answer publish MCVE, there is way too much information missing – Slava Apr 06 '16 at 14:57
  • What is MCVE exactly? Constructors and deconstructors? I tried to google it real quick but I'm still unsure, sorry :p – Wezabi Apr 06 '16 at 15:04
  • It's a [Minimal, Complete, and Verifiable Example](http://stackoverflow.com/help/mcve). Asking for it means that we need to see constructors, copy-constructor and destructors. – Some programmer dude Apr 06 '16 at 15:06
  • Does `RobotCustomer` have an appropriate copy constructor and copy assignment operator? – NathanOliver Apr 06 '16 at 15:06
  • I can update with constructors, but I don't have a copy constructor or a copy assignment operator. Is that why RobotCustomer dada's pointer gets dropped in dequeue possibly? – Wezabi Apr 06 '16 at 15:13
  • If you use `new` with `m_reqServices` then yes you need a proper copy constructor and copy assignment operator. See Joachim Pileborg's answer for why. – NathanOliver Apr 06 '16 at 15:23

1 Answers1

0

Lets look at a few of the lines in your function:

Node<T>* tempNode = new Node<T>;
tempNode = m_front;
delete tempNode;

First you allocate memory and assign its pointer to the tempNode variable. Then you reassign that variable to point to some other memory, losing the original pointer. Then you try to free the memory, which is no longer the original memory you allocated.

This should not cause the crash (as fas as I can see), but it is a memory leak.


My guess as to what is causing the crash is that you probably allocate memory dynamically in the RobotCustomer constructor, for the m_reqServices member. But if you don't implement a copy-constructor or copy-assignment operator, or just do a shallow copy of the pointer in those functions, then with the assignment

newNode->m_data = x

in the enqueue function you will have two object with the same pointer, and when one of them is deleted its destructor deletes the allocated memory, leaving the other objects pointer now pointing to unallocated memory, leading to undefined behavior when you try to free it again.

You need to read about the rules of three, five and zero. My recommendation is to use std::vector or std::array (as applicable) and simply follow the rule of zero.

Of course, talking about using the standard containers makes me wonder why you don't use std::queue to begin with?

Some programmer dude
  • 400,186
  • 35
  • 402
  • 621
  • Oh yea I was messing around with different options, I was going to change Node* tempNode = new Node; to Node* tempNode; – Wezabi Apr 06 '16 at 15:15
  • When I get input from the user, I do use the line 'm_reqServices = new string[m_numServices];' after verifying input. I guess in this test file I never do that though so m_reqServices is empty. I do concur with the "no copy-constructor or assignment operator" leading to a problem still though. As for using std::queue It's a homework assignment where we have to use out own queue class derived from an ADT abstractQueue that we're given. – Wezabi Apr 06 '16 at 15:22