CODEWITHSUNDEEP
c++classvector
msk
msk

Reputation: 139

Segmentation fault when operating a vector in c++

I am trying to learn c++, and i wanted to inicialize a vector of X instances as a class member with a simple program, but I am getting segmentation fault... Can you help?

#include <iostream>
#include <vector>

class X {
    int _x;
    public:
    X(int i) { _x = i; }
    void print() { std::cout << this->_x << std::endl; }
    void xAdd() { _x++; }
};


class Y {
    std::vector<X*> _x;
    public:
    Y(int size) : _x(size) {
        for (int i = 0; i < size; ++i) _x.push_back(new X(1));
    }
    void printAll() {
        for(unsigned int i = 0; i < _x.size()-1; i++) {
        _x[i]->print();
        }
    }
};

int main(){
    Y *y = new Y(5);
    y->printAll();
    return 0;
}

Upvotes: 1

Views: 717

Answers (3)

Zac Howland
Zac Howland

Reputation: 15872

You have 2 memory leaks. Do not use new unless you have to.

You are using loops for initialization when you do not need to.

You set the initial size (and thus, the initial values) of the vector, and then do push_back. Thus, the first N values are default constructed (and NULL).

Your printAll function will print all but the last element.

class X 
{
private:
    int _x;
public:
    X(int i) { _x = i; }
    void print() { std::cout << _x << std::endl; } // this-> is not needed
    void xAdd() { _x++; }
};

class Y 
{
private:
    std::vector<X> _x; // no need to store pointers, store the object itself
public:
    Y(int size) : _x(size, X(1)) // use the fill version of the constructor 
    { }

    // simple way to print (there are other ways to do the same thing)
    void printAll() 
    {
        std::for_each(_x.begin(), _x.end(), [](const X& x)
        {
            x.print();
        });
    }
};

int main()
{
    Y y(5); // no need for heap allocation
    y.printAll();
    return 0;
}

Upvotes: 1

AliciaBytes
AliciaBytes

Reputation: 7429

Your problem is in the constructor of your Y class:

class Y {
    std::vector<X*> _x;
    public:
    Y(int size) : _x(size) {  // initializing the vector with size elements all set to nullptr
        for (int i = 0; i < size; ++i) _x.push_back(new X(1)); // pushing back size pointers to actual instances of X
    }
    void printAll() {
        for(unsigned int i = 0; i < _x.size()-1; i++) { // iterating over the first size items of the vector which are the nullptrs and derefencing them.
        _x[i]->print();
        }
    }
};

you should think about making it a std::vector<X> to get rid of all the pointers you have to deal with at the moment.

Upvotes: 1

Mike Seymour
Mike Seymour

Reputation: 254461

You initialise _x with size null pointers; then you push another size valid pointers onto it. Then printAll tries to dereference those null pointers.

Either remove the initialiser (possibly adding _x.reserve(size); to minimise allocations); or change the loop body to _x[i] = new X(1);

As a general note, you're using new far too much. There's no reason for the vector to contain pointers rather than objects, or for y to be dynamic rather than automatic.

Upvotes: 4

Related Questions