Segmentation fault when using vectors in the class and constructor

Question

I was doing a list of programming projects, and this project is to make a 15 puzzle (slide puzzle). I was working on the project when I hit a small roadblock.

My code compiles just fine, but when I run it, I get a segmentation fault at line 12: pos[0] = x;

#include <iostream>
#include <vector>
#include <stdlib.h>
#include <time.h>
using namespace std;
class Tile{
private:
    vector<int> pos;
    int value;
public:
    Tile(int x, int y, int value_){
        pos[0] = x;
        pos[1] = y;
        value = value_;
    }
    ~Tile(){}
    int getPos(int a){return pos[a];}
    void setPos(int a, int b){pos[a] = b;}
};
int main(){
    Tile tile1(1, 2, 10);
    Tile* t1;
    t1 = &tile1;

    // returns position "x"
    cout << t1->getPos(0);
    return 0;
}

I mean, I could just do the whole project without having to use vectors/arrays to handle the position, but I do still want to know, for my own understanding in the future, why this doesn't work.

Based on the debug that I ran, the program is having trouble initializing the value of the pos[] vector.

Another issue: probably related, I tried setting the size of the vector when it was instantiated.

vector<int> pos(2);

But then I get the debug error:

error: expected identifier before numeric constant

Not sure whats going on here. I've tried a bunch of different things but I can't seem to figure out why my vectors don't work inside of classes.

I'm sure there are a hundred ways I could have done this little piece better, and I would love to know how you would have fixed it, but I also need to know what is wrong, specifically in the context of what I have written and tried.

Thanks.

Answer 1

There are couple of issue in the given code, which I have resolved and added comment in the code.

Issue in setPos and getPos might raise segmentation fault must be handle.

Added checks for the same.

#include <iostream>
#include <vector>
#include <stdlib.h>
#include <time.h>
using namespace std;
class Tile{
private:
    vector<int> pos;
    int value;
public:

    Tile(int x, int y, int value_){
        pos.push_back(x); // this is equivalent to pos[0] = x, in this case 
        pos.push_back(y); // this is equivalent to pos[0] = y, in this case
        value = value_;
    }

    ~Tile(){}

    int getPos(int a){
        if(a >= pos.size()){
            return -1; // if a is greater than size then pos[a] will raise the segmentation fault
        }
        return pos[a];
    }
    void setPos(int a, int b){
        if(a >= pos.size()){
            pos.resize(a+1); // to avoid segmentation fault, we are increasing the size if the given index is higher
            // resize initialise the value with 0 as default value.
        }
        pos[a] = b;
    }
};
int main(){
    Tile tile1(1, 2, 10);
    Tile* t1;
    t1 = &tile1;

    // returns position "x"
    cout << t1->getPos(0);
    return 0;
}

Answer 2

As mentioned in other answers, your vector is empty and your code is attempting to assign non-existent elements.

The solution is to always use initialisers instead of assignment. Rewrite your constructor as follows:

Tile(int x, int y, int value) :
    pos{x, y},
    value{value} {}

Note that the constructor body is now empty. All initialisation happens where it should — in the initialiser list.

Apart from that, your class does not need an explicitly defined destructor; the default destructor works just fine.

There are other issues with this class — for instance, what happens when the user does tile.setPos(3, 4)? A rule of thumb of good API design is to make it impossible to misuse the API.

Here’s how I would write your Tile class instead:

struct Tile {
    int x;
    int y;
    int value;

    Tile(int x, int y, int value) : x{x}, y{y}, value{value} {}
};

The getter and setter in your case wasn’t really doing any meaningful work. There’s an argument to be made to hide all data members behind accessors to future-proof access control. I’m no longer convinced this is actually useful but just in case, here’s a solution with that, too:

class Tile {
    int x_;
    int y_;
    int value_;

public:
    Tile(int x, int y, int value) : x_{x}, y_{y}, value_{value} {}

    int x() const { return x; }
    int& x() { return x; }

    int y() const { return y; }
    int& y() { return y; }

    int value() const { return value; }
};

This makes x and y readable and writable (via assignment: t.x() = 42;), and value only readable. Other APIs are possible, with different sets of trade-offs. The important thing is to be consistent.

Answer 3

I tried setting the size of the vector when it was instantiated.

vector<int> pos(2);

But then I get the debug error:

error: expected identifier before numeric constant

That's a compilation error, not a debug error.

You can't initialise members like that. However, you can (and should) initialise them using the parent constructor:

Tile(int x, int y, int value_)
    : pos(2)
{
    pos[0] = x;
    pos[1] = y;
    value = value_;
}

Currently you're just leaving your vector empty then accessing (and writing to!) elements that don't exist.

You really don't want a vector for this, anyway: that's a lot of dynamic allocation. How about a nice array? Or just two ints.

Answer 4

Your constructor doesn't set the size, so when you try to access/modify its contents, you are probably getting the exception.

Tile(int x, int y, int value_) : pos(2) {
    pos[0] = x;
    pos[1] = y;
    value = value_;
}

You can use the initialization list of the constructor to call the vector's constructor, as in the code above.