C++ “Access violation reading location” Error

Question

I just implemented a Destructor and I am getting an “Access violation reading location”. I beleive the problem is in my while loop but just can't figure it out.

Below is my code. If needing to reference any other part of my List Class please let me know.

Thanks!

List::List():first(NULL), last(NULL), nodeListTotal(0)
{
}    

List::~List()
{
    Node* currentNode = first;

    while( currentNode != 0 ) 
    {
        Node* temp = currentNode->getNext();
        delete currentNode;
        currentNode = temp;
    }

    first = 0;
}

Here is my entire List class. I have made the changes recommended, removed the first = 0; and change 0 to nullptr

#include "Node.h"
#include <string>
using namespace std;

class List
{
    private:
        int nodeListTotal;
        Node* first;
        Node* last;

    public:
        //Constructor
        List();
        //Destructor
        ~List();
        //Copy-Constructor
        //List(const List& theList);
        //////Overloading Assignment Operator
        //List& operator=(const List& L);

        void push_back(Node*);
        void push_front(Node*);
        Node* pop_back();
        Node* pop_front();
        Node* getFirst() const;
        Node* getLast() const;
        int getListLength() const;
};

List::List():first(NULL), last(NULL), nodeListTotal(0)
{
}

// Destructor
List::~List()
{
    Node* currentNode = first;

    while( currentNode != nullptr ) 
    {
        Node* temp = currentNode->getNext();
        delete currentNode;
        currentNode = temp;
    }
}

// Copy-Constructor
//List::List(const List& theList)
//{
//  Node * tempPtr = new Node;
//  tempPtr = theList.first;
//  List(tempPtr);
//  
//  while (tempPtr != NULL)
//  {
//      Node * copyNode = new Node;
//      copyNode = tempPtr;
//      tempPtr = tempPtr->getNext();
//      nodeListTotal++;
//  }
//}

// Overloading Assignemnt Operator
//List& List::operator=(const List& L)
//{
//  List* overList;
//  Node* temp = L.first;
//  
//  while( temp != NULL ) {
//      overList->getLast();
//      temp = temp -> getNext();
//      
//      return *this;
//}

void List::push_back(Node* newNode)
{
    Node* temp = last;
    if (temp)
        temp->setNext(newNode);
    else
        first = newNode;

    last = newNode;
    nodeListTotal++; 
}

void List::push_front(Node* newNode)
{
    Node* temp = getFirst();
    newNode->setNext(temp);
    first = newNode;
    nodeListTotal++;

    if (!temp)
        last = first;
}

Node* List::pop_back()
{
    Node* old = last;
    if (first == last)
    {
        first = 0;
        last = 0;
    }
    else
    {
        Node* temp = first;

        for (int i = 0; i < (nodeListTotal - 1); i++)
        {
            temp = temp->getNext();
        }

        temp->setNext(NULL);

        last = temp;
    }

        nodeListTotal--;
        return old;
}

Node* List::pop_front()
{
    Node* temp = getFirst();
    first = temp->getNext();

    if (!first)
        last = 0;

    nodeListTotal--;

    return temp;
}

Node* List::getFirst() const
{
    return first;
}

Node* List::getLast() const
{
    return last;
}

int List::getListLength() const
{
    return nodeListTotal;
}

Node.h

#include <string>
using namespace std;

class Node
{
    private:
        string dataItem;
        string dataUnit;
        int unitTotal;
        Node* next;

    public:
        //Constructor
        Node();

        Node(int, string, string);

        string getDescription( )const; 
        void setDescription(string);

        string getQuantityName()const; 
        void setQuantityName(string);

        int getQuantityNumber()const; 
        void setQuantityNumber(int);

        Node* getNext( )const; 
        void setNext(Node*);
};

Node::Node(void):dataItem("None"), dataUnit("None"), unitTotal(0), next(NULL)
{
}

Node::Node(int q, string i, string u):dataItem(i), dataUnit(u), unitTotal(q), next(NULL)
{
}

string Node::getDescription( ) const
{
    return dataItem;
}

void Node::setDescription(string iSetter)
{
    dataItem = iSetter;
}

string Node::getQuantityName() const
{
    return dataUnit;
}

void Node::setQuantityName(string uSetter)
{
    dataUnit = uSetter;
}

int Node::getQuantityNumber() const
{
    return unitTotal;
}

void Node::setQuantityNumber(int tSetter)
{
    unitTotal = tSetter;
}

Node* Node::getNext() const
{
    return next;
}

void Node::setNext(Node* nSetter)
{
    next = nSetter;
}

Driver.cpp

int main( )
{
    //===============================================
    // PART ONE
    //===============================================
    cout << "\nPart I: push_front and pop_front\n";
    cout << "\n----------------------------------\n";
    List groceries;

    // test push_back function
    groceries.push_front(new Node(1, "gallon", "milk") );
    groceries.push_front(new Node(2, "loaves", "bread") );
    groceries.push_front(new Node(1, "dozen", "eggs" ) );
    groceries.push_front(new Node(1,  "package", "bacon") );

    cout << "\nThe original nodes in the List:\n";
    printList(groceries);
    cout << "\n----------------------------------\n";

    // test push_front function
    cout << "\nAdding to the front of the List:\n";
    cout << "\n----------------------------------\n";
    groceries.push_front(new Node(2, "lbs", "hamburger") );
    groceries.push_front(new Node(1, "dozen", "hamburger buns") );

    printList(groceries);
    cout << "\n----------------------------------\n";

    // test pop-front
    cout << "\nRemoving the first node from the list.\n";
    cout << "\n----------------------------------\n";
    Node* item = groceries.pop_front( );
    cout << "\nPopped " << item->getDescription( ) << " from the list.\n\n";
    printList(groceries);
    if (item != NULL)
        delete item;

    // ===============================================
    // PART TWO: Uncomment this block to test part two
    // ===============================================

    cout << "\n----------------------------------\n";
    cout << "\nPart Two: Push_back and pop_back";

    // test push_back
    groceries.push_back(new Node(2, "cans", "orange juice") );
    groceries.push_back(new Node(1, "lb", "swiss cheese") );

    cout << "\nAdding two nodes at the end\n";
    cout << "\n----------------------------------\n";
    printList(groceries);

    // test pop-back
    cout << "\n----------------------------------\n";
    cout << "\nRemove last node from the list\n";
    cout << "\n----------------------------------\n";
    item = groceries.pop_back( );
    cout << "\nPopped " << item->getDescription( ) << " from the list.\n\n";

    printList(groceries);
    if (item != NULL)
        delete item;
    // ============================================
    // end of part two
    // ============================================

    // ================================================
    // PART THREE: uncomment this block to test part three
    // ================================================
    /*
    // create a second list to test assignment
    cout << "\n\n--------------extra credit------------------\n";
    cout << "\n\n overloaded assignment operator\n";
    cout << "The hardware list ...\n";
    cout << "\n-------------------------------------------\n";
    List hardware;
    hardware.push_back(new Node(2, "lbs", "nails") );
    hardware.push_back( new Node(3, "gals", "white paint") );
    hardware.push_back(new Node(1, "piece", "plywood") );
    printList(hardware);
    hardware = groceries;
    cout << "\n-------------------------------------------\n";
    cout << "\nafter assignment";
    cout << "\n-------------------------------------------\n";
    printList(hardware);

    cout << "\n-------------------------------------------\n";
    cout << "\nTest the copy constructor\n";
    cout << "\n-------------------------------------------\n";
    printFirstNode(hardware);

    // ==============================================
    // end of part 3
    // ==============================================
    */
    cout << "\n-------------------------------------------\n";
    cout << "\nEnd of Test";
    cout << "\n-------------------------------------------\n";
    system("PAUSE");
    return 0;
}

Answer 1

Looks like pop back does not remove last node from the list, but returns it. Then the node is deleted and in list's destructor you try to delete it second time.

Answer 2

The push_back() method could be your culprit, coupled with the failure to initialize Node:next to zero. If only one node is added to the list using push_back then the value of that node's next member would be unknown and in the destructor an attempt to delete a second node referring to a random memory location would cause the access error. Either ensure that the nodes values are initialized, or ensure that node.next is set explicitly in push_back() if it's the first node added to the list.

Something to note on pop_back() and pop_front(). Calling pop_back() on an empty list would still decrement nodeListTotal. Calling pop_front() on an empty list would actually cause an access violation trying to access address 0.

Answer 3

The below deletion of node is causing the problem while cleaning up in the destructor.

if (item != NULL)
    delete item;

When you do the pop_back you're deleting that(last) node in main(), but what happens to the node which is before that? it was pointing to the one which you deleted and is not set to NULL.

So, in the destructor when you start deleting the nodes you're checking for the NULL value of the nodes. All goes fine but now for the last one next wasn't set to NULL, instead it is still pointing to the one which you just deleted. Hence, tries to free the node which is already freed.

Then your code crashes.

Set the previous node's next to NULL whenever you're freeing the successive node.