Daisy-chaining function calls on pointers

Question

I encountered a scenario recently whilst working with a student, and I'm struggling to understand why the following example is failing.

I have a pointer to an object Game, and Game itself has a pointer to vector<Pair>. The failing line is the last line of of main(), where I am daisy-chaining methods:

gamePointer->getPairs()->push_back(pair);

In the above line, getPairs() returns a vector<Pair>*, and then push_back() is called to add a new Pair to the vector. This results in a read access violation. Interesting, swapping out the Game's vector<Pair> with a string, say, allows me to write the following, and it works:

gamePointer->getPairs()->append("B");

I've simplified the problem and reproduced a full example:

#include "pch.h"
#include <iostream>
#include <string>
#include <vector>

using namespace std;

class Pair 
{
private:
    string previous;
    string next;

public:
    Pair();
    Pair(string previous, string next);

    string getPrevious();
    string getNext();

    void setPrevious(string previous);
    void setNext(string next);
};

class Game 
{
private:
    vector<Pair>* pairs;
public:
    Game();

    vector<Pair>* getPairs();
    void setPairs(vector<Pair>* pairs);
};


Pair::Pair()
{
    this->setPrevious("a");
    this->setNext("b");
}

Pair::Pair(string previous, string next)
{
    this->setPrevious(previous);
    this->setNext(next);
}

string Pair::getPrevious()
{
    return this->previous;
}

string Pair::getNext()
{
    return this->next;
}

void Pair::setPrevious(string previous)
{
    this->previous = previous;
}

void Pair::setNext(string next)
{
    this->next = next;
}

Game::Game()
{
    vector<Pair> pairs;
    pairs.reserve(10);

    this->setPairs(&pairs);
}

vector<Pair>* Game::getPairs()
{
    return this->pairs;
}

void Game::setPairs(vector<Pair>* pairs)
{
    this->pairs = pairs;
}

int main()
{
    Game game;
    Game* gamePointer = &game;

    Pair pair("Previous", "Next");
    gamePointer->getPairs()->push_back(pair);
}

Answer 1

Game::Game()
{
    vector<Pair> pairs; // DANGER!
    pairs.reserve(10);

    this->setPairs(&pairs); // ARGHH!
} // < pairs dies on this line

The vector named pairs only lives while the constructor is running. You store a pointer to this, but the pointed-to-object immediately goes out of scope!

Instead, just make the member a vector instead of a pointer:

class Game 
{
private:
    vector<Pair> pairs; // the vector itself is a member of Game

You could then make getPairs like this:

vector<Pair>* Game::getPairs() // return a pointer
{
    return &pairs;
}

or this:

vector<Pair>& Game::getPairs() // return a reference
{
    return pairs;
}

---

What you are doing currently is Undefined Behaviour - this means your program is illegal, and anything could happen, including appearing to work normally.

"Appearing to work normally" is what you are seeing when you swap the vector for a string - your code is still broken, you just don't notice!

---

I can make an educated guess about why that happens, but this is in no way guaranteed.

vector behaviour:

• The vector object itself is on the stack, but it has to allocate a buffer on the heap using new.
• It then deletes this buffer when the vector goes out of scope at the end of Game::Game().
• The vector object itself is no longer valid, but the memory just happens to not get overwritten before you next try to use it.
• You try to use the (no longer alive) vector, and the memory still happens to contain a pointer to the buffer. The buffer has been released, so you get a "read access violation" when trying to access it.

string behaviour:

• The string does not have a to allocate a buffer. It is a valid implementation of std::string for it to use a "Small String Optimisation", where small strings (let's say, up to 16 characters, for example) are stored directly inside the string object itself, rather than in an allocated buffer.
• Therefore the string, including the actual content, are on the stack.
• The string object goes out of scope at the end of Game::Game(), but the memory just happens to not get overwritten before you next try to use it.
• You try to use the (no longer alive) string, and the memory still happens to contain the valid "short string" magic.
• Because this is on the stack, not the heap, the memory hasn't actually been released. So trying to access it does not cause a "read access violation".
• It is still totally illegal though!

Answer 2

Looking at the constructor of Game:

Game::Game()
{
    vector<Pair> pairs;
    pairs.reserve(10);

    this->setPairs(&pairs);
}

pairs is a local variable. It will be destroyed at the end of the constructor. Meaning this->pairs is a dangling pointer. This can be fixed by simply allocating this->pairs in the constructor directly:

Game::Game()
{
    this->pairs = new vector<Pair>;
    this->pairs->reserve(10);
} 

If you do this, always provide a deconstructor to clean up the allocated data:

Game::~Game()
{
    delete this->pairs;
}

This allows you to get rid of setPairs. In the end the class should look like this:

class Game
{
private:
    vector<Pair>* pairs;
public:
    Game();
    ~Game();

    vector<Pair>* getPairs();
};

To be honest, I don't really like your way of mixing classes like vector and raw pointers. In this simple example you don't even need a pointer at all. See BoBTFish answer on how you could make a pointer-less implementation. If you are, however, absolutely obligated to use pointers, use std::unique_ptr<std::vector<Pair>> or std::shared_ptr<std::vector<Pair>> instead. Please also consider getting rid of using namespace std; <>.