Segmentation fault on list.begin()

Question

I have this member function in my Folder class:

string _recFullPath() {
 list<Folder*> folders;
 list<Folder*>::iterator it = folders.begin();
 folders.push_front(this);
 it = folders.begin();
 while((*it)->hasParent()) {
  folders.push_front((*it)->parent());
  it = folders.begin();
 }
 folders.push_back(this);
 for(it = folders.begin(); it != folders.end(); ++it) {
  cout << (*it)->getName() << "/";
 }
}

This does compile, but when it comes to it = folders.begin(), in the while loop it gives a segmentation fault, and I cannot figure out why. The layout for a Folder object is this:

class Folder {
  private:
    Folder* _parent;
    string _name;
    string _fullPath;
    string _recStrFullPath;
    bool _hasParent;

  public:
    Folder(string name) {
        this->_name = name;
        this->_hasParent = false;
    }

    Folder(string name, Folder* parent) {
        this->_parent = parent;
        this->_name = name;
        this->_hasParent = true;
    }

    Folder* parent() {
        return this->_parent;
    }

    string getName() {
        return this->_name;
    }

};

And of course the above mentioned function. Can someone see what I'm doing wrong in the above code?

Answer 1

I don’t know why your while loop uses the iterator at all. This would be cleaner and simpler:

list<Folder*> folders;
Folder* current = this;

while (current->hasParent()) {
    folders.push_front(current);
    current = current.parent();
}

folders.push_front(current);

for(list<Folder*>::const_iterator i = folders.begin(); i != folders.end(); ++i) {
    cout << (*i)->getName() << "/";
}

Answer 2

It's not normally good form to require the use of pointers in a class without managing the storage in that class. At very least, you would have to make it very clear just how allocation requirements of the caller are supposed to work. Here's some code to illustrate:

Folder foo(){
    Folder bar("bar");
    Folder baz("baz", &bar);
    return baz;
}

What happens here is pretty ugly, but it looks like you did what you were supposed to. What happens at the return is that baz gets copied into the storage location it needs to go to for the caller, but baz is retaining a pointer to bar. bar (and the original baz, you have a copy now) are gone, freed from the stack at the end of the function.

There are a few ways to get out of this mess. The right way is probably to manage memory completely in the class itself. here's an alternative version:

class Folder {
  private:
    Folder* _parent;
    string _name;
    string _fullPath;
    string _recStrFullPath;
    bool _hasParent;

  public:
    Folder(const Folder & src)
        : _name(src._name), _fullPath(src._fullPath)
        , _recStrFullPath(src._recStrFullPath)
        {
        if (src._parent) {
            _parent = new Folder(src._parent);
        }
    }
    ~Folder() {
        delete _parent;
    }

    Folder(string name) {
        this->_name = name;
        this->_hasParent = false;
    }

    Folder(string name, const Folder & parent) {
        this->_parent = new Folder(parent);
        this->_name = name;
        this->_hasParent = true;
    }

    Folder* parent() {
        return this->_parent;
    }

    string getName() {
        return this->_name;
    }

};

The important changes are that instead of taking a pointer when making a child node, you actually duplicate the parent node into the child. the child has its' very own copy. It also looks after that copy so the caller doesn't have to care about it at all.

To make this work, some changes to the class were necessary. The call signature of the child forming constructor was changed to make it clear that the parent is not affected. When filling in the _parent, a copy gets made with new.

To facilitate this, it was necessary to add another constructor, a copy constructor, since we need to care for the _parent node specially.

Finally, since we are doing these allocs in the class itself, its necessary to add a destructor to clean up those allocs when the instance goes away.

Now, the caller can look like this:

Folder foo(){
    Folder bar("bar");
    Folder baz("baz", bar);
    return baz;
}

and work politely.