cliffclimber
cliffclimber

Reputation: 23

Why am I getting a segmentation fault error whenever I'm trying to run this LinkedList delete function?

I am trying to create a program to delete a node at the N'th position of a linked list. According to the output, it should be:

George
Betty
Felix
Renee

George 
Betty
Felix

George
Felix

Felix

When running on repl.it (my submission website), it brings up that I am getting a segmentation error. However, when I run it off my personal computer on CodeBlocks, it runs without errors, however, it only outputs the first line which is George, Betty, Felix, and Renee without deleting and re-outputting.

Here's my code:

#include <iostream>
#include <string>

using namespace std;

class Node {
  public:
    string name;
    Node* next;
};

Node* head;

class LinkedList {
  public:
    LinkedList();
    ~LinkedList();
    void push(string);
    void output();
    void remove(int);

  private:
    Node *first;
};


void LinkedList::remove(int n)
{
  struct Node* temp1 = head;
  if(n == 1)
  {
    head = temp1 -> next;
    delete(temp1);
    return;
  }
  int i = 0;
  for(i = 0; i < n - 2; i++)
  {
    temp1 = temp1 -> next;
  }
  struct Node* temp2 = temp1 -> next;
  temp1 -> next = temp2 -> next;
  delete(temp2);

}


LinkedList::LinkedList()
{
  first = NULL;
}

LinkedList::~LinkedList()
{
  Node *current=first;

  while(current!=NULL)
  {
    Node *ptr=current;
    current = current->next;
    delete(ptr);
  }
}

void LinkedList::push(string data)
{
  Node *temp;

  temp = new Node;
  (*temp).name = data;
  (*temp).next = first;
  first = temp;
}

void LinkedList::output()
{
  Node *current = first;

  while(current!=NULL)
  {
    cout << (*current).name << endl;
    current = (*current).next;
  }
  cout << endl;
}

int main() {
  LinkedList students;

  students.push("Renee");
  students.push("Felix");
  students.push("Betty");
  students.push("George");


  students.output();

  students.remove(3);
  students.output();

  students.remove(1);
  students.output();

  students.remove(0);
  students.output();


}

Upvotes: 2

Views: 64

Answers (2)

Vlad from Moscow
Vlad from Moscow

Reputation: 311078

For starters it is unclear what is this declaration

Node* head;

doing in the program.

The class Node should be declared within the class LinkedList and should be inaccessible for users of the list.

In C++ indices start from 0. So it would be better if you would count indices in the list also starting from 0. The type of the parameter n of the function remove should have unsigned integer type for example the type size_t.

You did not check within function whether a current pointer is equal to nullptr. So the function has undefined behavior.

Moreover instead of the data member first the function refers to the global variable head.

struct Node* temp1 = head;

It seems you copied and pastes somewhere (in the internet) a C code of singly linked list ant tried to update it.

The code of the function can be very simple if to use pointer to pointer. For example

void LinkedList::remove( size_t n )
{
    Node **current = &first;

    while ( n-- != 0 && *current != nullptr )
    {
        current = &( *current )->next;
    }

    if ( *current != nullptr )
    {
        Node *tmp = *current;
        *current = ( *current )->next;
        delete tmp;
    }
}

Indices passed to the function start from 0 as it should be in a C++ program.

Here is a demonstrative program.

#include <iostream>
#include <string>
#include <functional>

class LinkedList 
{
public:
    LinkedList() = default;
    LinkedList( const LinkedList & ) = delete;
    LinkedList & operator =( const LinkedList & ) = delete;

    ~LinkedList()
    {
        while ( head )
        {
            delete std::exchange( head, head->next );
        }           
    }

    void push( const std::string &s )
    {
        head = new Node { s, head };
    }

    bool remove( size_t n )
    {
        Node **current = &head;
        while ( n-- != 0 && *current != nullptr )
        {
            current = &( *current )->next;
        }

        bool success = *current != nullptr;

        if ( success )
        {
            delete std::exchange( *current, ( *current )->next );
        }

        return success;
    }

    friend std::ostream & operator <<( std::ostream &os, const LinkedList &list )
    {
        for ( Node *current = list.head; current != nullptr; current = current->next )
        {
            os << current->name << " -> ";
        }

        return os << "null";
    }

private:
    struct Node
    {
        std::string name;
        Node* next;
    } *head = nullptr;
};

int main() 
{
     LinkedList students;

    students.push("Renee");
    students.push("Felix");
    students.push("Betty");
    students.push("George");

    std::cout << students << '\n';

    students.remove(3);
    std::cout << students << '\n';

    students.remove(1);
    std::cout << students << '\n';

    students.remove(0);
    std::cout << students << '\n';

    return 0;
}

Its output is

George -> Betty -> Felix -> Renee -> null
George -> Betty -> Felix -> null
George -> Felix -> null
Felix -> null

Upvotes: 0

WhozCraig
WhozCraig

Reputation: 66244

Your code for everything but LinkedList::remove manages the list via the first member variable. But LinkedList::remove references head, a suspiciously otherwise-unused global variable. I'm confident that shouldn't even be in the code at all.

Delete the global head, and change LinkedList::remove to be:

void LinkedList::remove(int n)
{
    Node **pp = &first;
    while (*pp && n-- > 1)
        pp = &(*pp)->next;

    if (*pp)
    {
        Node *tmp = *pp;
        *pp = tmp->next;
        delete tmp;
    }
}

Upvotes: 2

Related Questions