Linked list assignment operator

Question

I wrote some code for the overloaded = operator for a linked list, but it's not doing anything for some reason and I can't figure out why.

the class containing the linked list is called String, with the struct ListNode being the nodes themselves.

ListNode:

struct ListNode
{
      char info;
      ListNode * next;
      ListNode(char newInfo, ListNode * newNext)
        : info( newInfo ), next( newNext )
      {
      }
};

String:

class String {

    private:
        ListNode* head;

    public:
    String( const char * s = "");
    String( const String & s );
    String operator = ( const String & s );
    ~String();
}
ostream & operator << ( ostream & out, String& str );
istream & operator >> ( istream & in, String & str );

String.cpp:

String::String( const char * s) {
    if (s == "") {
        head = NULL;
        return;
    }

    ListNode* newNode = new ListNode(s[0], NULL);
    head = newNode;
    ListNode* current = head;
    for (int i = 1; s[i] != 0; current = current->next) {
        current->next = new ListNode(s[i], NULL);
        ++i;
    }
}

String::String(const String& s ) {

    ListNode* current = new ListNode((s.head)->info, NULL); //make all next's null just in case
    head = current;
    for(ListNode* sstart = s.head->next; sstart != NULL; sstart = sstart->next) {
            current->next = new ListNode(sstart->info, NULL);
            current = current->next;
    }

}

//RETURN STRING BY REFERENCE OR COPY CONSTRUCTOR IS CALLED
String& String::operator = ( const String & s ) {
    ListNode* start = head;
    ListNode* tmp;
    while(start != NULL) {
        tmp = start->next;
        delete start;
        start = tmp;
    }

    head = NULL;

    if (s.head == NULL)
        return *this;    
    ListNode* current = new ListNode((s.head)->info, NULL); //make all next's null just in case
    head = current;
    for(ListNode* sstart = s.head->next; sstart != NULL; sstart = sstart->next) {
            current->next = new ListNode(sstart->info, NULL);
            current = current->next;
    }

    return *this;

}

String::~String() {
    ListNode* nextNode = head;
    ListNode* tmp;
    while(nextNode) {
        tmp = nextNode->next;
        delete nextNode;
        nextNode = tmp;
    }
}

ostream & operator << ( ostream & out, String& str) {

    for (int i = 0; i < str.length(); ++i) {
        out << str[i];
    }
    return out;

}

istream & operator >> ( istream & in, String & str ) {
    int len = in.gcount();
    char* buf = new char[len];
    char inChar;
    for(int i = 0; in >> inChar; ++i) {
        buf[i] = inChar;        
    }
    String tmp(buf);
    str = tmp;
}

In the first loop, I'm deleting the linked list pointed to by head. After that, I'm setting head to NULL for the case where s contains nothing at all. If it isn't, then, I set current to be a copy of the first ListNode in s and store current in head (if I traverse using head, then I lose the pointer to the start of the list). Finally, my 2nd loop will "attach" the rest of s to current.

When I run my code, nothing happens. my terminal will print out a blank line, and then nothing, suggesting to me that I'm probably going infinite somewhere. What is wrong with my code?

EDIT: changed deletion of linked list of this, issue still persists.

In this case, there's a pretty glaring error evident in your code. However, it's generally better (though not always necessary) to provide a complete, compilable example. Often the problem is not where you think it is. For instance, we're left to assume you're actually (correctly) dynamically allocating memory, since there's no `new` despite the `delete`. — jerry, Feb 08 '14 at 03:38
For one thing, in the first loop, you're accessing deleted memory (ie. delete start includes deleting start's "next" pointer. Should be "tmp=start->next; delete start; start=tmp;" — racraman, Feb 08 '14 at 03:42
One suggestion for next time -- write the primitive functions for your linked list such as Add(), Insert(), Remove(), etc. Then the assignment operator becomes a trivial function of just a few lines. — PaulMcKenzie, Feb 08 '14 at 03:50

jerry · Answer 1 · 2014-02-08T03:39:24.130

4

You are causing undefined behavior by accessing an object that has already been deleted (since tmp == start) in this code snippet:

tmp = start;
delete start;
start = tmp->next;

There may be other problems, but start by fixing that. For instance, you can store the next pointer in your temporary variable before deleting:

tmp = start->next;
delete start;
start = tmp;

edited Feb 08 '14 at 03:39

answered Feb 08 '14 at 03:34

jerry

2,581
1
21
32

Changed that, I still have the same issue. This time, I'm fairly certain it's because I messed up my 2nd for loop somewhere. EDIT: example I'm using is: String s("Hello"); String t("Goodbye"); s = t; – Jonathan Chiou Feb 08 '14 at 03:42
@JonathanChiou A few points: we don't know how you implemented your `String( const char *)` constructor, you didn't include any output statements in that snippet, we don't have any idea how the class writes itself out, and please edit your question and put the additional code there. – jerry Feb 08 '14 at 04:06
@JonathanChiou Please read http://stackoverflow.com/help/mcve an http://sscce.org/ and then give us a usable example. Your updated code is more complete, but is still missing some things and contains some syntax errors. Fixing those, I flushed out the code similar to how I guessed you had done it. The [result](http://ideone.com/FqqN8y) does print out what I would expect. By the way, there's at least one unrelated semantic error (don't check for an empty string in the `char *` constructor with `s==""`) – jerry Feb 09 '14 at 12:52

michaeltang · Answer 2 · 2014-02-08T05:27:16.360

1

I test the code you put in the question , it seems it is the problem of overloading << hope it will help

ostream & operator << ( ostream & out, String& str) {

    ListNode *p = str.head; // change head to public or define a public function get_head;
    while(p)
    {
        out << p->info;
        p = p->next;
    }

    return out;

}

edited Feb 08 '14 at 05:27

answered Feb 08 '14 at 03:51

michaeltang

2,850
15
18

I did have a copy constructor, but I didn't think it was relevant to my problem. – Jonathan Chiou Feb 08 '14 at 03:55
@JonathanChiou, is is convenient for you to put all your code in the question? – michaeltang Feb 08 '14 at 03:57
If you wrote a copy constructor, then why not just have your assignment operator call the copy constructor using the copy/swap idiom? Otherwise you're just duplicating code. – PaulMcKenzie Feb 08 '14 at 04:04
Assuming that you did write a copy constructor and you tested it thoroughly, here is the copy / swap idiom described: http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom Using this idiom, you no longer need to write all of that code in the assignment operator. – PaulMcKenzie Feb 08 '14 at 04:06

Linked list assignment operator

2 Answers2