Search code examples
c++pointersvectordereference

Segfault when dereferencing iterator for vector of pointers


I have a vector of object pointers

    std::vector<Element*> elements;

When iterating through the vector, I would like to double dereference the iterator in order to call the object's methods.

    std::cout << (*it)->getName() << std::endl;

This leads to a segfault. The relevant code is below.

I am thinking that the problem is with how I am initializing the vector, because I could move the for-loop to be in the method initialize() and it works fine. In takeTurn(), the vector is of the appropriate size and the pointers contain the correct addresses. Does this mean that the objects being pointed to are being prematurely destroyed?

main.cpp:

#include <vector>
#include <iostream>
#include "Element.h"

    std::vector<Element*> elements;

void initialize() {
    Element ice = Element("ice",1);
    Element fire = Element("fire",2);
    elements.push_back(&ice); 
    elements.push_back(&fire);
}

void takeTurn() {
    std::vector<Element*>::iterator it;
    for(it = elements.begin(); it != elements.end(); ++it) {
        std::cout << (*it)->getName() << std::endl;
    }
}

int main() {
    initialize();
    takeTurn();
    return 0;
}

Element.h:

#include <string>

class Element {
    public:
        Element(std::string name, int id);
        int getID() { return id_; }
        std::string getName() { return name_; }

    private:
        int id_;
        std::string name_;
};

Element.cpp:

#include "Element.h"

Element::Element(std::string name, int id) {
    name_ = name;
    id_ = id;
}

Solution

  • Your initialize function is broken. You create local objects, and then push their addresses onto the vector. But when the function returns, those objects are destroyed, and the pointers are no longer valid. The simplest fix, unless you need polymorphism, is to just make a vector of Element objects, instead of pointers.

    std::vector<Element> elements;
    ...
    elements.push_back(Element("ice",1));
    elements.push_back(Element("fire",2));
    

    If you need polymorphism, then use smart pointers.

    std::vector<std::unique_ptr<Element>> elements;
    ...
    elements.push_back(std::unique_ptr<Element>(new Element("ice",1)));
    elements.push_back(std::unique_ptr<Element>(new Element("fire",2)));
    

    If you were to continue to use raw pointers, then you would need some way to ensure the persistence of the objects, perhaps by allocating them with new. You would then need to ensure you call delete on each of those pointers you are done with them. I do not recommend this route.