Unique pointer in Linked List - Unhandled exception, stack overflow

Question

First of all, let me say thank you for all the help I've been received in the last couple hours. I've been struggle with this problem, how to convert from raw pointer to unique pointer and got myself into a lot of errors. However, with the help of this community, I've been thankful that my program finally compiles without errors at all. But I'm not there yet, I guess. I feel like I'm like one minute away from the finish line, so I won't give up till I can solve it. My program crashes as soon as it runs, it says stack overflow and throw out the exception. I guess it must be the way that I declare and initialize the unique pointer as a class member in the constructor is not correct at all and therefore it crashes right from the minute it calls the constructor. Would anyone please tell me what I should do to fix this error ? Thanks.

This is my main cpp file:

#include"ContactList.h"
#include<memory>

using namespace std;

int main()
{
    //ContactList* cl1 = new ContactList();

    unique_ptr<ContactList> cl1(new ContactList());
    string name;

    while(true)
    {
        cout << "Enter a name or q to quit: " << endl;
        cin >> name;
        if(name == "q")
            break;
        cl1->addToHead(name);
    }

    cl1->PrintList();
    return 0;
}

ContactList.h

#pragma once
#include"Contact.h"
#include<memory>

using namespace std;

class ContactList
{
public:
    ContactList();
    void addToHead(const std::string&);
    void PrintList();

private:
    //Contact* head;
    unique_ptr<Contact> head;
    int size;
};

ContactList.cpp

#include"ContactList.h"
#include<memory>

using namespace std;

ContactList::ContactList(): head(new Contact()), size(0)
{
}

void ContactList::addToHead(const string& name)
{
    //Contact* newOne = new Contact(name);
    unique_ptr<Contact> newOne(new Contact(name));

    if(head == 0)
    {
        head.swap(newOne);
        //head = move(newOne);
    }
    else
    {
        newOne->next.swap(head);
        head.swap(newOne);
        //newOne->next = move(head);
        //head = move(newOne);
    }
    size++;
}

void ContactList::PrintList()
{
    //Contact* tp = head;
    unique_ptr<Contact> tp(new Contact());
    tp.swap(head);
    //tp = move(head);

    while(tp != 0)
    {
        cout << *tp << endl;
        tp.swap(tp->next);
        //tp = move(tp->next);
    }
}

Contact.h

#pragma once
#include<iostream>
#include<string>
#include<memory>

class Contact
{
    friend std::ostream& operator<<(std::ostream& os, const Contact& c);
    friend class ContactList;

public:
    Contact(std::string name = "none");

private:
    std::string name;
    //Contact* next;    
    std::unique_ptr<Contact> next;
};

Contact.cpp
#include"Contact.h"

using namespace std;

Contact::Contact(string n):name(n), next(new Contact())
{
}

ostream& operator<<(ostream& os, const Contact& c)
{
    return os << "Name: " << c.name;
}

This is the error I get:

Unhandled exception at 0x77E3DEFE (ntdll.dll) in Practice.exe: 0xC00000FD: Stack overflow (parameters: 0x00000001, 0x002B2F58).

Answer 1

You have a problem in your ContactList::PrintList() method: you don't need unique_ptr when you iterate through some items just observing them.
When observing items, raw pointers are just fine.

In general, owning raw pointers are not good (except in some special cases), but observing raw pointers are just fine.

---

In addition, note also that in your ContactList default constructor, you don't need to allocate an empty Contact with new and assign it to head unique_ptr data member: unique_ptr default constructor will automatically initialize head to a nullptr.

Note also that the ContactList::PrintList() methods should be marked as const for proper const-correctness, since usually printing the content of some collection should not alter the items in the collection.

Finally, the ContactList allocation in your main() function can be simply done on the stack:

ContactList cl;

There's no need to use unique_ptr in this case (please program in C++, not in Java or C# style).

And, a style note: I don't like that some methods start with upper-case letter (e.g. PrintList()) and others with lower-case letter (e.g. addToHead()): choose one style, and be coherent with it (at list at the source file level, if not at the whole project level).

---

Below there's a single source file test code, based on your code with some fixes applied.

I compiled it and tested a bit with VC10 (Visual Studio 2010 SP1); it seems to work:

C:\Temp>test.exe
Enter a name or q to quit:
Bob
Enter a name or q to quit:
Jane
Enter a name or q to quit:
John
Enter a name or q to quit:
Mary
Enter a name or q to quit:
q
[Contact name: Mary]
[Contact name: John]
[Contact name: Jane]
[Contact name: Bob]

---

Compilable source code follows:

#include <iostream>
#include <memory>
#include <ostream>
#include <string>
using namespace std;

// "Imaginary" Contact implementation (you didn't provide it)
struct Contact {
    Contact() {}
    explicit Contact(const string& n) : name(n) {}

    string name;    
    unique_ptr<Contact> next;
};

ostream& operator<<(ostream& os, const Contact& c) {
    os << "[Contact name: " << c.name << "]";
    return os;
}

class ContactList {
public:
    ContactList();
    void AddToHead(const string&);
    void PrintList() const;

private:
    unique_ptr<Contact> head;
    int size;
};

ContactList::ContactList()
  : size(0) {
  // No need to initialize head pointer.
  // It will be automatically initialized to nullptr.
}

void ContactList::AddToHead(const string& name) {
    unique_ptr<Contact> newOne(new Contact(name));
    if(head == 0) {
        head.swap(newOne);
    } else {
        newOne->next.swap(head);
        head.swap(newOne);
    }
    size++;
}

void ContactList::PrintList() const {
    const Contact * pContact = head.get();   
    while (pContact != nullptr) {
        cout << *pContact << endl;
        pContact = pContact->next.get();
    }
}

int main() {
    // No need to allocate ContactList using unique_ptr.
    // Stack scoped-based allocation is just fine.
    ContactList cl;    

    while (true) {
        cout << "Enter a name or q to quit: " << endl;
        string name;
        cin >> name;
        if (name == "q")
            break;
        cl.AddToHead(name);
    }

    cl.PrintList();
}

Answer 2

You didn't post the code for Contact, but I assume it's the same as in one of your previous questions:

Contact::Contact(string n):name(n), next(new Contact())
{
}

As you can see, constructing a Contact requires setting its next member to a new Contact.
In order to construct that Contact, you're going to create a new Contact for its next member.
And so on, et cetera, to infinity and beyond.

This is the cause of of the stack overflow - Contact construction never ends.

You probably don't want next to be anything in particular for a newly constructed Contact, so try

Contact::Contact(string n):name(n), next(0)
{
}