CODEWITHSUNDEEP
c++classvectorsegmentation-fault
Sardonyx
Sardonyx

Reputation: 55

Segmentation Fault when operating on retrieved vector

I'm kinda new to C++, and I have a question that I am curious about. For a while I was getting segmentation faults, though I did eventually make it work, but I would like to know why it wasn't before. This is what I had:

#include <string>
#include <sstream>
#include <iostream>
#include <vector>
using namespace std;
class A {
    private:
        int num;
    public:
        A(int i){this->num=i;}
        int getNum(){return this->num;}
};
class B {
    private:
        vector<A*> list;
    public:
        vector<A*> getList(){return this->list;}
        void addA(A* i){this->getList().push_back(i);}
        string as_a_string(){
            stringstream result;
            cout << "Flag1" <<endl; //only for debug, this prints
            for (vector<A*>::iterator x = this->getList().begin(); x != this->getList().end(); ++x) {
                cout << "Flag2" << endl; //only for debug, this prints
                A* w = *x;
                cout << "Flag3" << endl; //only for debug, this prints
                result << w->getNum() << " ";
                cout << "Flag4" << endl; //only for debug, this does not print
            }
            return result.str();
        }
};
int main() {
    A* a = new A(4);
    B* b = new B();
    b->addA(a);
    cout << b->as_a_string() << endl;
    return 0;
}

And I solved my problem by replacing every instance of this->getList() with this->list (there were 3; one in B::addA(A*) and two in the for loop definition in B::as_a_string()). Why would using the member itself instead of accessing it through a method affect the workings of this program?

Upvotes: 2

Views: 85

Answers (3)

sbaker
sbaker

Reputation: 537

Your code:

class B {
    private:
        vector<A*> list;
    public:
        vector<A*> getList(){return this->list;}
        void addA(A* i){this->getList().push_back(i);}

is calling push_back() on a temporary because getList() returns by value. After calling add(), the member variable list is unchanged. You can change your getList() function signature to return a reference like this:

vector<A*>& getList(){return this->list;}

Honestly, you probably are better off with the solution that you used to avoid the problem because providing a public accessor that returns a reference to your implementation details is something you would generally want to avoid.

Upvotes: 2

greatwolf
greatwolf

Reputation: 20838

The problem is that your B::getList() returns list by value and so a copy of B::list is actually returned rather than the original list itself.

In your loop here:

for (vector<A*>::iterator x = this->getList().begin();
     x != this->getList().end(); ++x)

You start by getting a begin iterator to this temporary list. Everytime the condition is checked, you are comparing it to the end iterator to a different temporary list object.

The easy fix for this is to have B::getList() return the list by reference:

vector<A*>& getList();

Upvotes: 2

Mats Petersson
Mats Petersson

Reputation: 129364

Since getList is returning a temporary copy of list, the list is gone again when you try to iterate through it.

There are a few different ways to deal with this:

  1. Just use list (no need to use this-> - it is not PHP, Python or some such language where you always have to refer to members via the this/self pointer).
  2. Let getList return a reference to the vector. (vector<A*>& getList() { ... };)
  3. Use a local variable as a copy

However, option 3, that won't help in addA, which requires that you operati on the ORIGINAL list - as it stands, your push_back operate on a temporary copy of list, which is immediately destroyed afterpush_back returns - so it doesn't actually add anything at all to the actual list member variable).

Personally, I would change your internal code to use list directly, and keep getList returning a copy, so that if some external function wants to get hold of the list, it can.

Edit: You are also leaking memory by not deleing the A* and B* objects that you create in main.

Upvotes: 2

Related Questions