2

There is a segmentation fault in the remove function of this doubly linked list. Debugging with gdb in the linux terminal shows that
1) Only the first condition, if(curr->prev == NULL), where the node to delete is the first node in the list is ever reached.
2) On the command delete curr, the program steps into the destructor for the class that models the data object, and breaks on the command delete title.

Edit:

After implementing user6545984 answer, the only issue is a segementation fault on tail = tail->prev in the else if(curr->next == NULL) condition. The program is unable to get past this conditional statement.
Also, additional code has been added below. I apologize for the length.

Code for the add and remove functions, as well as the for the destructor:

Add Function (creates doubly linked list)

 void SongList::addSongSorted(const Song &aSong)
    {
        char title[MAX_CHAR];
        char currTitle[MAX_CHAR];
        aSong.getTitle(title);

        Node * newNode = new Node(aSong);
        Node * prev = NULL;
        Node * curr = head;

        while(curr)
        {
            curr->song.getTitle(currTitle);

            if(strcmp(title, currTitle) < 0)

                break;

            prev = curr;
            curr = curr->next;
        }
        newNode->next = curr;
        newNode->prev = prev;     //Added due to answer. See edit note.

        if(!prev)

            head = newNode;

        else
        {
            prev->next = newNode;
        }

        size++;
    }

Remove function

void SongList::remove(const Song& aSong)
{
    Node *curr = head;

    int indexRem = 0;
    int j = 0;
    //get the index to remove
    cout << "Enter the index to remove.\n";
    cout << "This value can be obtained via the list all   functionality.\n";
    cin >> indexRem;
    while(indexRem > getSize()|| indexRem < 0)
    {
        cout << "Enter a valid index to remove.\n";
        cin >> indexRem;
    }
    while(curr && j < indexRem)
    {
        curr=curr->next;
        j++;
    }

    if(!curr)
    {
        cout << "didnt find anything" << endl;
    }

    else
    {
       if(curr->prev == NULL)      //NOTE: this is the only condition ever engaged
       {

        head = head->next;
        head->prev = NULL;
        delete curr;            //NOTE: goes to destructor
        curr = NULL;
        }
        else if(curr->next == NULL)
        {
            tail = tail->prev;      //EDIT: only issue is seg fault here
            tail->next = NULL;
            delete curr;
            curr = NULL;
        }
        else
        {
            Node * previous = curr->prev;
            Node * following = curr->next;
            previous->next = following;
            following->prev = previous;
            delete curr;

        }
    }

Destructor (please now ignore)

    Song::~Song()
        {
            if(title != NULL)
                delete [] title;    //Breaks here
            if(artist != NULL)
                delete [] artist;
            if(album != NULL)
                delete [] album;
        }

Header file for object class

#ifndef SONG_H
#define SONG_H
#include<cstring>
#include<stdio.h>
#include<cstdlib>
const int MAX_CHAR = 101;
const int SONG_CAPACITY = 100;

class Song
{
public:
    //constructors
    Song();
    Song(const char title[], const char artist[], const char album[], int min, int sec);

    //Destructor
    ~Song();

    //accessor functions
    void getTitle(char title[]) const;
    void getArtist(char artist[]) const;
    void getAlbum(char album[]) const;
    int getMin()const;
    int getSec()const;
    void print() const;

    //mutator functions
    void setTitle(const char title[]);
    void setArtist(const char artist[]);
    void setAlbum(const char album[]);
    void setMin(int &min);
    void setSec(int &sec);


private:
    char*    title;
    char*    artist;
    char*    album;
    int     min;
    int     sec;
};


#endif

Source file for object class

    #include "song.h"
    #include <iostream>
    using namespace std;


    Song::Song()
    {
        title = new char[strlen("no title")+1];
        strcpy(title, "no title");
        artist = new char[strlen("no artist")+1];
        strcpy(artist, "no artist");
        album = new char[strlen("no album")+1];
        strcpy(album, "no album");
        min = 0;
        sec = 0;
    }


    Song::Song(const char title[], const char artist[], const char album[], int min, int sec)
    {
        this->title = new char[strlen(title)+1];
        strcpy(this->title, title);
        this->artist = new char[strlen(artist)+1];
        strcpy(this->artist, artist);
        this->album = new char[strlen(album)+1];
        strcpy(this->album, album);
        this->min = min;
        this->sec = sec;
    }

    Song::~Song()
    {
        if(title != NULL)
            delete [] title;
        if(artist != NULL)
            delete [] artist;
        if(album != NULL)
            delete [] album;
    }

    void Song::getTitle(char title[]) const
    {
        strcpy(title, this->title);
    }

    void Song::getArtist(char artist[]) const
    {
        strcpy(artist, this->artist);
    }

    void Song::getAlbum(char album[]) const
    {
        strcpy(album, this->album);
    }

    int Song::getMin()const
    {
        return min;
    }

    int Song::getSec()const
    {
        return sec;
    }


    void Song::print() const
    {
       cout << title << '\t'
            << artist << '\t'
            << album << '\t'
            << min << '\t'
            << sec << endl;
    }

    void Song::setTitle(const char title[])
    {
        if(this->title != NULL)
            delete [] this->title;
        this->title = new char[strlen(title)+1];
        strcpy(this->title, title);
    }

    void Song::setArtist(const char artist[])
    {
        if(this->artist != NULL)
            delete [] this->artist;
        this->artist = new char[strlen(artist)+1];
        strcpy(this->artist, artist);
    }

    void Song::setAlbum(const char album[])
    {
        if(this->album != NULL)
            delete [] this->album;
        this->album = new char[strlen(album)+1];
        strcpy(this->album, album);
    }

    void Song::setMin(int &min)
    {
        this->min = min;
    }

    void Song::setSec(int &sec)
    {
        this->sec = sec;
    }

###Header file for object management class

    #ifndef SONG_LIST
    #define SONG_LIST
    #include "song.h"
    #include <iostream>
    using namespace std;

    class SongList
    {
    public:
        SongList();
        SongList(const char fileName[]);

        ~SongList();
        bool get(int index, Song& aSong) const;
        //Search function declaration
        void searchArtist(const char artist[], Song& match) const;
        void searchAlbum(const char album[], Song& match) const;
        int getSize() const;
        void printAll() const;
        void saveToFile(const char fileName[]) const;

        void addSong(const Song& aSong);
       // void addAtStart(const Song& aSong);
       // void append(const Song& aSong);
        void addSongSorted(const Song& aSong);
        void loadFromFile(const char fileName[]);
        void remove(const Song& aSong);

    private: 
        struct Node
        {
            Song    song;
            Node *  next;
            Node *  prev;

            Node(const Song& aSong)
            {
                char    title[MAX_CHAR];
                char    artist[MAX_CHAR];
                char    album[MAX_CHAR];
                int     min;
                int     sec;

                aSong.getTitle(title);
                aSong.getArtist(artist);
                aSong.getAlbum(album);
                min =  aSong.getMin();
                sec =  aSong.getSec();

                song.setTitle(title);
                song.setArtist(artist);
                song.setAlbum(album);
                song.setMin(min);
                song.setSec(sec);
                next = NULL;
            }
        };

        Node * head;
        Node * tail;
        int size;
    };

    #endif

###Source file for object management class
#include "songlist.h"
#include <iostream>
#include <fstream>
#include <iomanip>
#include <cstring>

using namespace std;

//Ok as is
SongList::SongList()
{
    head = NULL; 
    tail = NULL;   
    size = 0;
}

//OK as is
SongList::SongList(const char fileName[])
{
    head = NULL;
    tail = NULL;
    size = 0;
    loadFromFile(fileName);
}

//Needs modification to deconstruct tail...
SongList::~SongList()
{
    Node * curr = head;

    while(head != NULL)
    {
        curr = head->next;
        delete head;
        head = curr;
    }
}
//OK as is
void SongList::loadFromFile(const char fileName[])
{
    ifstream        in;
    char            title[MAX_CHAR];
    char            artist[MAX_CHAR];
    char            album[MAX_CHAR];
    int             min;
    int             sec;
    Song    aSong;

    in.open (fileName);
    if(!in)
    {
        in.clear();
        cerr << endl << "Fail to open " 
        << fileName << " for input!" << endl << endl;
        exit(1);
    }
    //cout << "loading point reached" << endl;
    in.getline(title, MAX_CHAR, ';');
    while (!in.eof())
    {
        //cout << "check input from file" << endl;
        in.getline(artist, MAX_CHAR, ';');
        in.getline(album, MAX_CHAR, ';');
        in >> min;
        in.ignore(MAX_CHAR, ';');
        in >> sec;
        in.ignore(MAX_CHAR, '\n');

        aSong.setTitle(title);
        aSong.setArtist(artist);
        aSong.setAlbum(album);
        aSong.setMin(min);
        aSong.setSec(sec);
        addSong(aSong);
     // aSong.print();

        in.getline(title, MAX_CHAR, ';');
    }
    in.close();
}

//Ok as is
int SongList::getSize()const
{
    return size;
}

bool SongList::get(int index, Song &aSong) const
{
    char    title[MAX_CHAR];
    char    artist[MAX_CHAR];
    char    album[MAX_CHAR];
    int     min;
    int     sec;

    if(index < 0 || index >= size)
        return false;

    int i;
    Node * curr = head;
    for(i = 0; i < index; i++)
    {
        curr = curr->next;
    }

    curr->song.getTitle(title);
    curr->song.getArtist(artist);
    curr->song.getAlbum(album);
    curr->song.getMin();
    curr->song.getSec();
    aSong.setTitle(title);
    aSong.setArtist(artist);
    aSong.setAlbum(album);
    aSong.setMin(min);
    aSong.setSec(sec);
    return true;
}
//OK as is 
void SongList::searchArtist(const char artist[], Song& match) const
{
    Node * curr;
    char    currentTitle[MAX_CHAR];
    char    currentArtist[MAX_CHAR];
    char    currentAlbum[MAX_CHAR];
    int     currentMin;
    int     currentSec;

    for(curr = head; curr != NULL; curr = curr->next)
    {
        curr->song.getTitle(currentTitle);
        curr->song.getArtist(currentArtist);
        curr->song.getAlbum(currentAlbum);
        curr->song.getMin();
        curr->song.getSec();
        if(strcmp(artist, currentArtist) == 0)
        {
            cout << "Found: " << currentTitle << '\t' 
                              << currentArtist << '\t'
                              << currentAlbum << '\t'
                              << currentMin << ':' 
                              << currentSec << endl;
        }
    }
}

//Ok as is
void SongList::searchAlbum(const char album[], Song &match) const
{
    Node * curr;
    char    currentTitle[MAX_CHAR];
    char    currentArtist[MAX_CHAR];
    char    currentAlbum[MAX_CHAR];
    int     currentMin;
    int     currentSec;

    for(curr = head; curr != NULL; curr = curr->next)
    {
        curr->song.getTitle(currentTitle);
        curr->song.getArtist(currentArtist);
        curr->song.getAlbum(currentAlbum);
        curr->song.getMin();
        curr->song.getSec();
        if(strcmp(album, currentAlbum) == 0)
        {
            cout << "Found: " << currentTitle << '\t'
                              << currentArtist << '\t'
                              << currentAlbum << '\t'
                              << currentMin << ':'
                              << currentSec << endl;
        }
    }
}
//OK as is 
void SongList::printAll() const
{
    Node * curr;

    char    title[MAX_CHAR];
    char    artist[MAX_CHAR];
    char    album[MAX_CHAR];
    int     min;
    int     sec;
    int     index = 0;
    for(curr = head; curr != NULL; curr = curr->next)
    {

        curr->song.getTitle(title);
        curr->song.getArtist(artist);
        curr->song.getAlbum(album);
        min = curr->song.getMin();
        sec = curr->song.getSec();

        cout << index << " " << title << " "  
             << artist << " " << album << " " 
             << min << ':' << sec << endl;
        index++;
    }
}
//OK as is 
void SongList::saveToFile(const char fileName[])const
{
    ofstream    out;
    Node*       curr;
    char        title[MAX_CHAR];
    char        artist[MAX_CHAR];
    char        album[MAX_CHAR];
    int         min = 0;
    int         sec = 0;

    out.open(fileName);
    if(!out)
    {
        out.clear();
        cerr << endl << "Fail to open" << fileName << ". Check your directory." << endl << endl;
        exit(1);
    }

    for(curr = head; curr; curr = curr->next)
    {
        curr->song.getTitle(title);
        curr->song.getArtist(artist);
        curr->song.getAlbum(album);
        min = curr->song.getMin();
        sec = curr->song.getSec();
        out << title << ';' << artist << ';' << album << ';' << min << ';' << sec << endl;
    }

    out.close();
}

void SongList::addSong(const Song &aSong)
{
    addSongSorted(aSong);
}   
//Don't think I need this
/*
void SongList::addAtStart(const Song &aSong)
{
    Node * newNode = new Node(aSong);

    newNode->next = head;
    head = newNode;
    size++;
}
//...or this
void SongList::append(const Song &aSong)
{
    Node * newNode = new Node(aSong);

    if(!head)
        head = newNode;
    else
    {
        Node * curr = head;

        while(curr->next)
        {
            curr = curr->next;
        }

        curr->next = newNode;
    }
    size++;
}

*/

void SongList::addSongSorted(const Song &aSong)
{

    char title[MAX_CHAR];
    char currTitle[MAX_CHAR];
    aSong.getTitle(title);

    Node * newNode = new Node(aSong);

    Node * prev = NULL;
    Node * curr = head;

    while(curr)
    {
        curr->song.getTitle(currTitle);

        if(strcmp(title, currTitle) < 0)

            break;


        prev = curr;
        curr = curr->next;

    }
    newNode->next = curr;
    newNode->prev = prev;

    if(!prev)

        head = newNode;

    else
    {
        prev->next = newNode;
    }

   // printAll();

    size++;

}



void SongList::remove(const Song& aSong)
{
    Node *curr = head;

    int indexRem = 0;
    int j = 0;

    cout << "Enter the index to remove.\n";
    cout << "This value can be obtained via the list all functionality.\n";
    cin >> indexRem;
    while(indexRem > getSize()|| indexRem < 0)
    {
        cout << "Enter a valid index to remove.\n";
        cin >> indexRem;
    }
    while(curr && j < indexRem)
    {
        curr=curr->next;
        j++;
    }

    if(!curr)
    {
        cout << "didnt find anything" << endl;
    }

    else 
    {
       if(curr->prev == NULL)
       { 

        head = head->next;

        head->prev = NULL;
        delete curr;
        curr = NULL;
        }
        else if(curr->next == NULL)
        {
            tail = tail->prev;
            tail->next = NULL;
            delete curr;
            curr = NULL;
        }
        else
        {
            Node * previous = curr->prev;
            Node * following = curr->next;
            previous->next = following;
            following->prev = previous;
            delete curr;

        }
    }

    size--;
}

Any suggestions?

User1023
  • 23
  • 3
  • Off topic: `delete` transparently handles the NULL case. There's no need to test for it. – user4581301 Aug 10 '16 at 05:23
  • Off topic: Instead of ` curr->song.getTitle(currTitle); if(strcmp(title, currTitle) < 0)` consider adding a `bool Song::isTitle(const char * title)` method. Saves you some code, a string copy into `curTitle`, and probably `curTitle`. – user4581301 Aug 10 '16 at 05:31
  • 1
    On topic: does `Song` have a copy constructor and an assignment operator? If not, you're setting yourself up for a [Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) fail, and that often look exactly like what you are reporting here. – user4581301 Aug 10 '16 at 05:36
  • @user4581301 I should, here's a pastebin of the file for the object's class: http://pastebin.com/Mq7Bk9mU – User1023 Aug 10 '16 at 05:45
  • You have no copy constructor or assignment operator in that code. If `Song` is copied or assigned, you'll have two `Song`s pointing to the same `title` etc... `delete` one and the other is left with invalid pointers. – user4581301 Aug 10 '16 at 05:51
  • What does `Node` look like? What does it do with `aSong`? – kfsone Aug 10 '16 at 06:02
  • @kfsone Here's the header file for songlist.cpp, which is in the pastebin above: http://pastebin.com/K8r8guW2 – User1023 Aug 10 '16 at 06:04
  • Didn't tag this earlier. All information important to the question should be in the question, otherwise the question gets garbled when the off-site host garbage collects or shuffles links. – user4581301 Aug 10 '16 at 06:10
  • @user4581301 I will update the question immediately. – User1023 Aug 10 '16 at 06:18

2 Answers2

2

Your forget the ->prev pointer when add a new node, which makes if(curr->prev == NULL) always be true. Just add newNode->prev = prev; in function addSongSorted().

By the way, you may be want --size when remove a node.

bigbin
  • 56
  • 4
-1

It gives you an error because you should never use delete or delete[] without using new or new[]. New gives you some memory from heap during runtime, while char[size] gives you some memory during compilation, so the size of the variable is hard-coded in your code

desu
  • 455
  • 2
  • 18
  • I didn't include the code, but the elements being destructed are dynamically allocated cstrings, and thus `new` is used. Is this what you are talking about? – User1023 Aug 10 '16 at 05:11
  • @User1023 yes. So I'll try to find another problem – desu Aug 10 '16 at 05:19
  • Perhaps the manner in which the list is constructed does not mesh with the remove function? – User1023 Aug 10 '16 at 05:21