I am trying to implement a Bayesian estimator for the card game Avalon. The game has five rounds and each round contains at most five proposals made by five different players. If a proposal is accepted, players go on a quest and the game proceeds to the next round. Before the previous round is completed, it is not known which 5 players will get to propose teams in the next round. I wanted to keep track of the current player that gets to propose a team using an iterator, but somehow it ends up pointing to nowhere. Specifically, in the constructor called for round1, the iterator Round::proposer points correctly to &PlayerA, the beginning of Round::proposers. However, when I add this instance (or a copy of it?) to Game::rounds, then the Round member proposer of Game::rounds.back() points nowhere, even though the Round member proposers is still correct. Why does that happen? During execution, of course a read access violation exception is thrown during the call (*Round::proposer)->make_proposal();. I apologize for the lengthy question, but two levels of indirection seemed necessary to produce the error.
// Player.h
#include <string>
class Player
{
private:
std::string name;
public:
Player(std::string name) : name(name) {};
void make_proposal() const {};
};
// Round.h
#include "Player.h"
#include <vector>
class Round
{
private:
std::vector<const Player*> proposers;
std::vector<const Player*>::const_iterator proposer;
public:
Round(std::vector<const Player*> proposers) : proposers(proposers), proposer(Round::proposers.begin()) {};
void next_proposal() { (*Round::proposer)->make_proposal(); };
};
// Game.h
#include "Round.h"
#include <vector>
class Game
{
private:
std::vector<Player*> players;
std::vector<Player*>::iterator active_player;
std::vector<Round> rounds;
public:
Game(std::vector<Player*> players);
void advance_player();
void next_round();
};
// Game.cpp
#include "Game.h"
Game::Game(std::vector<Player*> players)
: players(players), active_player(Game::players.begin())
{
std::vector<Player*>::const_iterator player = Game::players.begin();
std::vector<const Player*> proposers = { *player };
for (unsigned int i = 0; i < 4; ++i) {
++player;
if (player == Game::players.end()) player = Game::players.begin();
proposers.push_back(*player);
}
Round round1(proposers);
Game::rounds = { round1 };
}
void Game::next_round()
{
Game::rounds.back().next_proposal();
}
#include <iostream>
#include "Game.h"
int main()
{
Player playerA("A");
Player playerB("B");
Player playerC("C");
Player playerD("D");
Player playerE("E");
Player playerF("F");
std::vector<Player*> players = { &playerA, &playerB, &playerC, &playerD, &playerE, &playerF };
Game game(players);
for(unsigned int i = 0; i < 5; ++i) {
game.next_round();
}
}
Surprisingly, replacing the two lines of code
Round round1(proposers);
Game::rounds = { round1 };
in Game.cpp with
Round* round1 = new Round(proposers);
Game::rounds = { *round1 };
fixes the issue, although I really don't understand why. After all, rounds is a member variable of Game and exists until instance game is destroyed. Follow-up question to this hack: is the instance to which round1 points in the last code snippet destroyed by the default constructor of class Game since it is dereferenced before adding to the member variable?
Your Round cannot be copied without problems:
class Round
{
private:
std::vector<const Player*> proposers;
std::vector<const Player*>::const_iterator proposer;
public:
Round(std::vector<const Player*> proposers) : proposers(proposers), proposer(Round::proposers.begin()) {};
void next_proposal() { (*Round::proposer)->make_proposal(); };
};
If you do copy it, the proposer will still be an iterator to an element in the original Round not to the vector in the copy. When you do this:
Round* round1 = new Round(proposers);
Game::rounds = { *round1 };
Then the local object round1 is not destroyed at the end of the scope, hence the iterator that is now inside rounds, after making a copy of round1, refers to an element that is still alive. Though it refers to the element inside round1, not to the Round that you placed in rounds.
Either take care of the rule of 3/5 for Round, or use indices instead of iterators. Indices get not invalidated when the whole vector is copied. (They also get not invalidated when you push back more elements to the vector, but iterators do)
A simpler example for similar problem:
#include <iostream>
struct broken {
int x;
int* ptr;
broken(int a = 0) : x(a),ptr(&x) {}
};
int main() {
broken a{42};
broken b{123};
a = b;
a.x = 0;
std::cout << *(a.ptr);
}
After copying b to a the pointer in a will still point to b.x, hence output is 123 (not 0 as one might expect).