Mysterious segfault when using stl list

Question

I'm getting a segfault on the following line:

 employees.push_back(Salesperson(id, fname, lname));

I don't have the slightest clue what could be causing it. All I'm trying to do is read a text file, put the information from it into a simple class with nothing but getters and setters for each value and a few comparison operators, and put objects of that class into a list. For some reason any time I try to insert into the list, it segfaults. I'm not trying to dereference the end iterator, and it makes no difference which of the lists insertion methods I use. All of them cause the same thing.

int main(int argc, char** argv)
{
    ifstream input;
    input.open("sales.txt");
    string s;
    istringstream ss;

    list<Salesperson> employees;

    while (getline(input, s))
    {
        ss.str(s);

        int year, id, sales;
        string fname, lname;

        ss >> year;
        ss >> id;
        ss >> fname;
        ss >> lname;
        ss >> sales;

        getline(input, s);
        ss.str(s);
        float sale;
        OrderedList<float> ol;

        for (int i = 0; i < sales; i++)
        {
            ss >> sale;
            ol.insert(sale);
        }


        list<Salesperson>::iterator it = find(employees.begin(), employees.end(), Salesperson(id, fname, lname));
        if (it == employees.end()) {
            employees.push_back(Salesperson(id, fname, lname));
            employees.back().setSales(employees.back().getSales() + ol);
        } else {
            it->setSales(it->getSales() + ol);
        }

        cout << it->getSales() << endl;

    }
    input.close();

    return 0;
}

Salesperson class

#ifndef SALESPERSON_H
#define SALESPERSON_H

#include <string>
class Salesperson
{
public:
    //Constructor
    Salesperson(int id, std::string fn, std::string ln)
    {
        employeeID = id;
        fname = fn;
        lname = ln;
    }

    //Gang of Three
    Salesperson(const Salesperson& orig) {*this = orig;}
    ~Salesperson() {}
    void operator=(const Salesperson& orig) 
    {
        employeeID = orig.employeeID;
        fname = orig.fname;
        lname = orig.lname;
        sales = orig.sales;
    }

    //Getters
    int getEmployeeID() {return employeeID;}
    std::string getFname() {return fname;}
    std::string getLname() {return lname;}
    OrderedList<float> getSales() {return sales;}

    //Setters
    void setEmployeeID(int a) {employeeID = a;}
    void setFname(std::string a) {fname = a;}
    void setLname(std::string a) {lname = a;}
    void setSales(OrderedList<float>& a) {sales = a;}

    //Operators
    bool operator<(Salesperson s) {return employeeID < s.employeeID;}
    bool operator==(Salesperson s) {return employeeID == s.employeeID;}
private:
    //Fields
    int employeeID;
    std::string fname, lname;
    OrderedList<float> sales;
};

OrderedList cpp

#include "Node.h"

template<class type>
OrderedList<type>::OrderedList(const OrderedList<type>& list)
{
    *this = list;
}

template<class type>
void OrderedList<type>::clear()
{
    for (Node<type>* i = head; i != NULL; head = i)
    {
        i = i->getLink();
        delete head;
    }
    size = 0;
}

template<class type>
void OrderedList<type>::remove(type item)
{
    Node<type>* temp = head, *prev = head;
    for (; temp != NULL; prev = temp, temp = temp->getLink())
        if (item == temp->getData()) break;

    if (temp != NULL)
    {
        if (prev == temp) head = head->getLink();
        else prev->setLink(temp->getLink());
        delete temp;
        size--;
    }
}

template<class type>
void OrderedList<type>::operator=(const OrderedList<type>& list)
{
    clear();
    Node<type>* tail = NULL;
    for (Node<type>* i = list.head; i != NULL; i = i->getLink())
    {
        if (head == NULL)
        {
            head = new Node<type > (i->getData(), NULL);
            tail = head;
        } else
        {
            tail->setLink(new Node<type > (i->getData(), NULL));
            tail = tail->getLink();
        }
    }
    size = list.size;
}

template<class type>
std::ostream& operator<<(std::ostream& out, const OrderedList<type>& list)
{
    out << "[";
    for (Node<type>* i = list.head; i != NULL; i = i->getLink())
    {
        out << i->getData();
        if (i->getLink() != NULL) out << ", ";
    }
    out << "]";

    return out;
}

template<class type>
void OrderedList<type>::insert(type d)
{
    size++;
    Node<type>* item = new Node<type>(d, NULL);

    Node<type> *i = head, *prev = NULL;
    while (i != NULL)
    {
        if (i->getData() >= d) break;
        prev = i;
        i = i->getLink();
    }

    if (prev == NULL)
    {
        item->setLink(head);
        head = item;
    } else {
        prev->setLink(item);
        item->setLink(i);
    }
}

template<class type>
type OrderedList<type>::get(int k) const
{
    if (k <= 0 || k > size) return NULL;

    Node<type>* i = head;
    type data;
    for (int j = 0; j < k; j++)
    {
        data = i->getData();
        i = i->getLink();
    }
    return data;
}

template<class type>
OrderedList<type> OrderedList<type>::kLargest(int k) const
{
    OrderedList list;
    Node<type>* i = head;

    if (k <= 0 || k > size) return list;

    for (int j = 0; j < size-k; j++)
    {
        i = i->getLink();
    }
    for (int j = 0; j < k; j++)
    {
        list.insert(i->getData());
        i = i->getLink();
    }
    return list;
}

template<class type>
OrderedList<type> OrderedList<type>::operator+(const OrderedList& list) const
{
    Node<type>* i = head;
    Node<type>* j = list.head;

    OrderedList newList;
    Node<type>* end;

    for(int k = 0; k < size + list.size; k++)
    {
        if(newList.size == 0 && i->getData() <= j->getData()) {
            newList.head = new Node<type>(i->getData(), NULL);
            end = newList.head;
            i = i->getLink();
            newList.size++;
            continue;
        } else if(newList.size == 0 && i->getData() <= j->getData()) {
            newList.head = new Node<type>(j->getData(), NULL);
            end = newList.head;
            j = j->getLink();
            newList.size++;
            continue;
        }

        if(i == NULL) {
            end->setLink(new Node<type>(j->getData(), NULL));
            end = end->getLink();
            j = j->getLink();
        } else if(j == NULL) {
            end->setLink(new Node<type>(i->getData(), NULL));
            end = end->getLink();
            i = i->getLink();
        } else if(i->getData() <= j->getData()) {
            end->setLink(new Node<type>(i->getData(), NULL));
            end = end->getLink();
            i = i->getLink();
        } else if(i->getData() > j->getData()) {
            end->setLink(new Node<type>(j->getData(), NULL));
            end = end->getLink();
            j = j->getLink();
        }

        newList.size++;
    }

    return newList;
}

Problem with `OrderedList` perhaps? Nothing obviously wrong with the posted code. — john, Apr 17 '13 at 06:21
I can't see anything obviously wrong, so the error is probably in either `OrderedList` or `Salesperson`. Do they both follow the [Rule of Three](http://stackoverflow.com/questions/4172722)? Are they both correctly copyable or movable? Does `OrderedList` manage its resources correctly, especially when you use its `operator+`? Perhaps you could post their definitions. — Mike Seymour, Apr 17 '13 at 06:25

NPE · Answer 1 · 2013-04-17T06:32:06.720

1

There is probably something wrong with the Salesperson class. It probably violates the Rule of Three or has another, similar, issue. Without seeing its code it's hard to be more specific.

edited Apr 17 '13 at 06:32

answered Apr 17 '13 at 06:18

NPE

486,780
108
951
1,012

regarding 2) why can it be executed when employess is empty? the line before pushes an element on the list. – Tobias Langner Apr 17 '13 at 06:29
@TobiasLangner: You are quite right, thanks. Will fix in a moment. – NPE Apr 17 '13 at 06:31
I've added the Rule of Three definitions to Salesperson and posted it, but I don't see why it needed them. – user1884814 Apr 17 '13 at 06:53
@user1884814: Now you've posted the definition, `Salesperson` should follow the "rule of zero" (i.e. just the implicitly declared destructor and copy constructor/assignment), since it doesn't manage any resources other than its own members. The problem is more likely to be in `OrderedList`, which does manage memory in quite a complicated way. – Mike Seymour Apr 17 '13 at 07:42
I've narrowed the problem to my Rule of Three in the OrderedList. I segfault sometimes when I try to use the copy constructor, but I can't tell exactly what conditions cause it. – user1884814 Apr 17 '13 at 07:46

score 0 · Answer 2 · answered Apr 17 '13 at 07:19

It's odd that your OrderedList<> template class is defined largely in a .cpp file - I don't know if that's anything to do with your problem or not.

However, the copy ctor:

template<class type>
OrderedList<type>::OrderedList(const OrderedList<type>& list)
{
    *this = list;
}

looks to have at least one problem.

Since a copy constructor is responsible for construction an object, the state of members that aren't initialized in the initializer list may not be well defined - particularly base types like pointers. You're trying to delegate the work of the copy to the assignment operator:

template<class type>
void OrderedList<type>::operator=(const OrderedList<type>& list)
{
    clear();
    Node<type>* tail = NULL;
    for (Node<type>* i = list.head; i != NULL; i = i->getLink())
    {
        if (head == NULL)
        {
            head = new Node<type > (i->getData(), NULL);
            tail = head;
        } else
        {
            tail->setLink(new Node<type > (i->getData(), NULL));
            tail = tail->getLink();
        }
    }
    size = list.size;
}

The first thing that the operator=() function does is call clear() - but clear() relies on the member head to be initialized to something sensible, which hasn't happened yet for the object under construction (since head is apparently a raw pointer). You should at least have a initialization list item for head (and maybe other members of OrderedList<>:

template<class type>
OrderedList<type>::OrderedList(const OrderedList<type>& list)
    : head(0)
{
    *this = list;
}

I've tried this but no luck. I am pretty sure now that the error is something close to this in the OrderedList Rule of Three. — user1884814, Apr 17 '13 at 07:33
What does a debugger tell you? Maybe also post the definition of `OrederedList<>`. — Michael Burr, Apr 17 '13 at 07:53

Mysterious segfault when using stl list

2 Answers2