c++ - Glorious Crashing while copying lists -

so i'm ready toss computer out of window, , i'm @ point figure should ask before destroy expensive piece of equipment.

my program has list filled in automatically (i manually input items), , outputs it. then, have block of code output again using assignment operators, , third copy copy constructor.

the assignment operator crashes program, if comment out let copy constructor, list comes out empty.

anything says "todo" can ignore, that's noted me fix later.

here action happens:

list.cpp

(this not main function, there's nothing wrong there)

    list::list() : m_pfront(0), m_pback(0), _pdata(0)     {}      list::~list()     {         clear();     }          list::list(const char* p)     {         if(!p)         {             _pdata = 0;             return;         }         if(strlen(p) == 0)         {             _pdata = 0;             return;         }         _pdata = new char[strlen(p) + 1];         strcpy(_pdata, p);     }      void list::clear()     {         //delete          if(!m_pfront)         {             return;         }         delete m_pfront;         node* p = m_pback;         //walking list         while(p)         {             //get pointer next in list             node* ptemp = p -> m_pnext;             delete p;             //move p along list             p = ptemp;         }         m_pfront = 0;         m_pback = 0;     }      void list::pushfront(std::string data)     {         //create new node         node* p = new node(data);          //empty list             if(!m_pfront)         {             m_pfront = p;             m_pback = p;         }         else //not empty list         {             p -> m_pnext = m_pfront;             m_pfront -> m_pprev = p;             m_pfront = p;         }     }      void list::pushback(std::string data)     {         node* p =  new node(data);          if(!m_pback)         {             m_pfront = p;             m_pback = p;                 }         else         {             p -> m_pprev = m_pback;             m_pback -> m_pnext = p;             m_pback = p;         }            }          void list::popfront()     {         if(m_pfront == 0)         {             //todo: need handle problem             return;         }         if(m_pback == m_pfront)         {             clear();             return;         }         node* p = m_pfront;         m_pfront = m_pfront -> m_pnext;         p -> m_pnext = 0;         m_pfront -> m_pprev = 0;             delete p;     }      void list::popback()     {         if(m_pback == 0)         {             //todo: need handle problem             return;         }         if(m_pback == m_pfront)         {             clear();             return;         }         node* p = m_pback;         m_pback = m_pback -> m_pprev;         p -> m_pprev = 0;         m_pback -> m_pnext = 0;         delete p;     }       ostream& list::output(ostream& os)     {         if(empty() == true)         {             os << "<empty>";         }         else         {             m_pfront -> outputnode(os);         }         return os;         }           std::string& list::back() const     {         if(m_pback == 0)         {             //todo: need handle problem         }         return m_pback -> getdata();     }      std::string& list::front() const     {         if(m_pfront == 0)         {             //todo: need handle problem         }     return m_pfront -> getdata();       }      //copy constructor     list::list(const list& str)     {         if(empty() == true)         {             _pdata = 0;             return;         }         _pdata = new char[strlen(str._pdata) + 1];         strcpy(_pdata, str._pdata);     }     //deep copy     list& list::operator=(const list& str)     {         if(&str == this)         {             return *this;         }         delete [] _pdata;         _pdata = new char[strlen(str._pdata) + 1];         strcpy(_pdata, str._pdata);         return *this;     } 

edit: list class made, if needs seen well

list.h

class list {     public:         list();         list(const char* p);         //copy constructor         list(const list& str);         //deep copy         list& operator=(const list& str);         ~list();         void clear();         //adds front          void pushfront(std::string data);         //adds         void pushback(std::string data);         //removes front         void popfront();         //removes         void popback();         //gets value         std::string& back() const;         //gets value         std::string& front() const;          bool empty() const {return m_pfront == 0;}          ostream& output(ostream& os);      private:             node* m_pfront;         node* m_pback;         char* _pdata;      }; 

so here's what's happening:

in copy constructor, check whether list empty. result of check undefined because m_pfront uninitialized, in debug build, chances check true. either way, since you're not copying nodes, _pdata, resulting list empty (with exception of _pdata maybe being set).

in assignment operator, before line _pdata = new char[strlen(str._pdata) + 1]; fail check if str._pdata points anything. if doesn't , you're doing strlen(0), crashes , burns.

my advice proper implementation of copy constructor. 1 deep copy, , implement copy , swap idiom assignment operator.

edit: example

the source code below implements small subset of list class question demonstrate deep copy constructor , assignment operator using copy , swap idiom mentioned in paragraph above.

before show source code, important realize deep copying list not easy. number of things have considered. have make copies of nodes. want make copies of data. maybe not deep copies. or maybe specific needs don't require copy of data @ all.

in case, list doubly linked. if node had copy constructor naive deep copy end stack overflow due endless chaining of copy constructor.

here's example of such naive implementation.

node(const node &other)  {     if (other.next)          next = new node(*other.next);     if (other.prev)          prev = new node(*other.prev); } 

in example, choose not implement copy constructor node clarity. choose make copies of data, in form of std::string match question.

list.h

#ifndef list_example_h_ #define list_example_h_  #include <string>  struct node {     std::string data;     node *next, *prev;     node(const std::string &d)          : next(0), prev(0), data(d)     {     } };  class list {     node *front;     node *back;     std::string data;  public:     list();     list(const std::string &);     list(const list &);     ~list();      list& operator=(list);      void clear();     void pushback(const std::string&);      bool empty() const { return front == 0; }      friend void swap(list &, list &);      void print(); };  #endif // list_example_h_ 

list.cc

#include "list.h" #include <iostream>  list::list()     : front(0), back(0), data() { }  list::list(const std::string &in)     : front(0), back(0), data(in) { }  list::~list() {     clear(); }  list::list(const list &other)     : front(0), back(0), data(other.data) {     if (!other.empty())         (node *node = other.front; node; node = node->next)              pushback(node->data); }  list& list::operator=(list other) {     swap(*this, other);     return *this; }  void list::clear() {     node *node = front;     while (node) {         node *to_delete = node;         node = node->next;         delete to_delete;     }     front = = 0; }  void list::pushback(const std::string &data) {     node *node = new node(data);     if (empty()) {         front = = node;     } else {         back->next = node;         node->prev = back;         = node;     } }  void list::print() {     std::cout << data << std::endl;     (node *node = front; node; node = node->next)         std::cout << node->data << std::endl;     std::cout << std::endl; }  void swap(list &first, list &second) {     using std::swap;     swap(first.front, second.front);     swap(first.back, second.back);     swap(first.data, second.data); }  int main() {     list a("foo");     a.pushback("a");     a.pushback("b");     a.pushback("c");     a.print();      list b("bar");     b.pushback("d");     b.pushback("e");     b.pushback("f");      list c(b);     c.print();      c = a;     c.print();     a.print();      return 0; } 

why assignment operator , swap function way explained better can in aforementioned answer describes copy , swap idiom. leaves implementation of copy constructor. let's @ line line.

1. list::list(const list &other) 2.     : front(0), back(0), data(other.data) 3. { 4.     if (!other.empty()) 5.         (node *node = other.front; node; node = node->next)  6.             pushback(node->data); 7. } 

1. our copy constructor takes reference const list. promise not change it.
2. we initialize our own members. since data not pointer might copy in initializer.
3. yeah, have add line proper markdown numbering.
4. if other list empty, we're done here. there no reason check whether list constructed empty. is.
5. for each node in other list... (sorry, markdown syntax again, doesn't let me combine 5 , 6 nicely).
6. ...we create new one. explained above, there's no copy constructor node in example, use our pushback method.
7. for completeness sake, line totally obvious.

looping on nodes in other list way not nicest. should prefer using iterator , calling pushback(*iter).