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).

I've just replaced the unique tag with unique_ptr, which seems more specific. — Mr.C64
– Mr.C64, Commented Oct 7, 2013 at 10:34
@FilipeGonçalves: Thanks, I just posted the Contact.h and Contact.cpp up — Hoang Minh
– Hoang Minh, Commented Oct 7, 2013 at 20:19

molbdnilo · Accepted Answer · 2013-10-07 10:06:54Z

3

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)
{
}

answered Oct 7, 2013 at 10:06

molbdnilo

67.1k3 gold badges46 silver badges90 bronze badges

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

2 Comments

Filipe Gonçalves Over a year ago

@HoangMinh are you sure? Did you recompile it all? The error mentioned by molbdnilo is the apparent cause of stack overflow.

molbdnilo Over a year ago

@HoangMinh "it did not work" is probably the least helpful problem description in the world.

Mr.C64 · Accepted Answer · 2013-10-07 10:31:38Z

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();
}

edited Oct 7, 2013 at 10:31

answered Oct 7, 2013 at 10:25

Mr.C64

43.3k15 gold badges97 silver badges170 bronze badges

3 Comments

Hoang Minh Over a year ago

Thank a lot for your help. I've learned so many things from your post. When you said that there is no need to use unique_ptr to allocate the ContactList in the main function; instead just do it in stack (aka ContactList cl1), how do you know when to allocate a variable/object in stack or heap ? From my understanding is, we need to put in heap if it's a large object. However, I have no idea to know which one is big and which one is small. So, I just play safe and use unique pointer for every object that I come across.

Mr.C64 Over a year ago

@HoangMinh: If you create a std::vector with 300,000 integers "on the stack", the actual memory for the vector values is actually allocated dynamically on the heap. But an instance of std::vector class is not that big: it may contain 3 pointers, occupying a total of 3x4 = 12 bytes on x86 32-bit architectures; this is just fine for stack allocations. Similarly, your ContactList contains a unique_ptr and a size data member, for a total of 4 + 4 = 8 bytes, which is just fine for the stack. A key point is that allocating on the stack is cheaper than allocating on the heap.

Mr.C64 Over a year ago

@HoangMinh: Continuing from my last comment, unless you don't have some particular reason to allocate on the heap using unique_ptr, consider allocating instances of C++ classes on the stack as the default option. That was certainly just fine for the case of ContactList inside main().

Collectives™ on Stack Overflow

Unique pointer in Linked List - Unhandled exception, stack overflow

2 Answers 2

2 Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related