Linked List overload operator = Run time error

Question

So I am really frustrated as to why this is happening. I am implementing a class that is similar to std::string but this is using Link List instead of arrays. my overloaded operator = is not working for some odd reason. below you can see, when I print the pointer inside the method the string is copied to the link list, however when I create a string object with this pointer to return, then the console prints out infinite junk. any ideas to what I am missing here ? (I am only pasting related code)

static int NumAllocations = 0;

struct ListNode {
    char info;
    ListNode *next;
    ListNode () : info('0'), next(0) {}
    ListNode ( char c ) : info (c), next(0) {}


};


class MyString {
 private:
    ListNode *head;
    static void delstring (ListNode *l);
    static int strlen (ListNode * head);
    static ListNode* strcpy (ListNode *dest, ListNode *src);
 public:

MyString::MyString () : head(0) {}

MyString::MyString (ListNode *l) : head(l) {}

MyString::MyString( const MyString & s ) {
    if (s.head == 0)
        head = 0;
    else
        head = strcpy(head, s.head);
}


MyString MyString::operator = (const MyString & s ){               
    ListNode *renew = NULL;
    if (head != s.head) {
        if (head != 0)
            delstring(this -> head);

        head = strcpy(head, s.head);
        // printList(head); (this prints out the string just fine, so it must be the                                   constructor ? but what about it ?!

        MyString res (head);
        return res;
    }
}


MyString::~MyString(){
    if (head == 0)
        return;
    ListNode *temp = NULL;
    do {
        temp = head -> next;
        delete head;
        -- NumAllocations;
        head = temp;
    } while (temp != 0);
}

STATIC PUBLIC FUNCTIONS

ListNode* MyString::strcpy (ListNode *dest, ListNode *src){
    dest = new ListNode (src -> info);
    ++ NumAllocations;
    ListNode *iter = dest;
    for (ListNode *ptr = src -> next; ptr != 0; ptr = ptr ->next){
        iter -> next = new ListNode (ptr -> info);
        iter = iter -> next;
        ++ NumAllocations;
    }
    return dest;
}


void MyString::delstring (ListNode *l){
    if (l == 0)
        return;
    ListNode *temp = NULL;
    do {
        temp = l -> next;
        delete []l;

        -- NumAllocations;
        l = temp;
    } while (temp != 0);
    l = 0;
}

Class definitions would assist this effort nicely. The node-management code even more-so. And I see zero-reason why dest is even supplied to the poorly named strcpy member. It is passed by value, and whatever it was is immediately lost on the first line, so it may as well be a local variable and not a parameter at all. That function looks like it may be a problem, so I'd start there. — WhozCraig
– WhozCraig, Commented Oct 18, 2013 at 4:12
I added the class definitions. strcpy is a member function of std::string and this class is to be as identical as possible to that, hence the name picking. like I mentioned the list is copied properly, because when i print it right before returning, it prints fine, it messes up when i try to create a new object with that pointer "head" in the last 2 lines of operator = method. — Tangleman
– Tangleman, Commented Oct 18, 2013 at 4:27

WhozCraig · Accepted Answer · 2013-10-18 05:04:14Z

2

Two things are fundamentally wrong with your assignment operator.

Not all control paths return values.
You shouldn't need the temp-final copy in the first place. The function should be returning a reference, specifically *this.

So...

MyString& MyString::operator = (const MyString & s )
{               
    if (head != s.head) 
    {
        if (head != 0)
            delstring(this -> head);
        head = strcpy(head, s.head);
    }
    return *this;
}

Also, everything I see in this code says the ListNode objects are allocated individually and linked together, yet in the delstring member you do this:

void MyString::delstring (ListNode *l)
{
    if (l == 0)
        return;
    ListNode *temp = NULL;
    do {
        temp = l -> next;
        delete []l;  // <<==== vector delete of single allocated item

        -- NumAllocations;
        l = temp;
    } while (temp != 0);
    l = 0;
}

Maybe try this instead:

void MyString::delstring (ListNode *& l)
{
    while (l)
    {
        ListNode *temp = l;
        l = l->next;
        delete temp;
        --NumAllocations;
    }
}

note this takes a pointer-reference rather than a pointer. it will set the caller's pointer to nullptr once the list is empty (assuming you properly terminated your list on construction, and it looks like you do).

edited Oct 18, 2013 at 5:04

answered Oct 18, 2013 at 4:45

WhozCraig

66.5k11 gold badges82 silver badges148 bronze badges

Sign up to request clarification or add additional context in comments.

7 Comments

Tangleman Over a year ago

I greatly appreciate your help, I am sorry that I didn't include more of the code, didn't want to over complicated the issue but I guess I made it worse. I do have a copy constructor written, it is now included as well as the destructor. I can clearly see your point on return this. I will try and see if that changes anything. (but I suspect that the destructor is doing something wrong, since printing the object inside this method (print (objhead)) prints the string, but in main, its a different story.

Tangleman Over a year ago

@WhosCraig you are awesome man, that function above works perfect. I still don't understand why building a new object was messing it up, but it was obviously wrong. thanks a lot for putting through reading all that messy code!

WhozCraig Over a year ago

@Tangleman there is still another issue, and it is likely related; improper vector destruction of the node allocations. I'll throw it in then leave it to you.

Tangleman Over a year ago

@WhosCraig haha alright if you just point it out and ill try to figure it out, but so far everything works, i still have more methods to do such as overload +=, +, >>, and things like find etc.

Tangleman Over a year ago

@WhosCraig Oh okay, I accidentally put "[]" before head, it meant to be delete head, just like the destructor, I will fix that also. again thanks for the help! hopefully the rest will be cake.

|

Collectives™ on Stack Overflow

Linked List overload operator = Run time error

1 Answer 1

7 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

7 Comments

Your Answer

Sign up or log in

Post as a guest

Related