I tried running the code on a few compilers and got different results. I either got some weird exit code like -1073741819 (0xC0000005) or free(): double free detected in tcache 2 or when debugging I got SIGTRAP (Trace/breakpoint trap) SIGSEGV (Segmentation fault).
I'm 99% sure the problem is that I'm freeing the ships array twice. But why do I not get errors when I don't run func() twice on both players or if I change anything else in the code? What is the problem exactly and how can I fix the error?
Yes I am using a dynamic array instead of a vector, because I'm trying to practise and learn. Yes I know this memory allocation method creates memory fragmentation, but I never change the size of the matrix, so it shouldn't be a problem.
Code:
struct point_t {
int x;
int y;
point_t();
};
point_t::point_t() {
this->x = 0;
this->y = 0;
}
struct ship_t {
int size;
point_t end_coords[2];
};
struct player_t {
int **map;
int map_size;
ship_t *ships;
int ships_count;
player_t(int, ship_t *, int);
~player_t();
};
player_t::player_t(int map_size, ship_t *ships, int ships_count) {
this->map = nullptr;
this->map_size = map_size;
this->ships = ships;
this->ships_count = ships_count;
}
player_t::~player_t() {
// Free map memory
// Free each column (sub-array)
for(int i = 0; i < map_size; i++) {
delete[] map[i];
}
// Free each row (array of pointers)
delete[] map;
map = nullptr;
map_size = 0;
delete[] ships;
ships = nullptr;
ships_count = 0;
}
void func(player_t &player) {
player.map = new int *[player.map_size];
for(int i = 0; i < player.map_size; i++) {
player.map[i] = new int[player.map_size];
for(int j = 0; j < player.map_size; j++) {
player.map[i][j] = 0;
}
}
}
int main()
{
ship_t *ships = new ship_t[(5 * 5) / 2];
for(int i = 0; i < 5; i++) {
ships[i].size = i + 1;
}
player_t player1 = player_t(5, ships, 5);
player_t player2 = player_t(5, ships, 5);
func(player1);
func(player2);
return 0;
}
Running your code with Visual Studio gives me:
HEAP CORRUPTION DETECTED: after Normal block (#72) at 0x00B065E8.
CRT detected that the application wrote to memory after end of heap buffer.
when doing the delete[] ships;.
The first problem is that you are trying to initialize points in the ships constructor, but:
this->end_coords[2] = {};
this statement is assigning a default initialized point to the third point in the end_coords member of ships. Unfortunately the array is two points large.
Moreover, since player_t destructor destroys ships, you are giving ownership of ships to player_t. But you are giving ownership of the same dynamic allocated array to two players:
int main()
{
ship_t *ships = new ship_t[(5 * 5) / 2];
for(int i = 0; i < 5; i++) {
ships[i].size = i + 1;
}
player_t player1 = player_t(5, ships, 5); // <-- ships
player_t player2 = player_t(5, ships, 5); // <-- ships
Not a good idea. The same ships array is used by two players, which makes no sense, since each player should have his own ships, I guess.
Finally you shall not call the destructor of player, because it will be called again on return (scope exit).
Comment out this line in the ship_t constructor:
//this->end_coords[2] = {};
Allocate two ships arrays, to give each player ownership:
ship_t *ships = new ship_t[(5 * 5) / 2];
for (int i = 0; i < 5; i++) {
ships[i].size = i + 1;
}
player_t player1 = player_t(5, ships, 5);
ship_t *ships2 = new ship_t[(5 * 5) / 2];
for (int i = 0; i < 5; i++) {
ships2[i].size = i + 1;
}
player_t player2 = player_t(5, ships2, 5);
Let's call structs with their names. Then use default initialization within the struct to avoid useless constructors. Remove all allocations unless really really needed. Let's use std::vector<>.
I'm not a fan of vectors of vectors for maps, but in this case it is really a simple solution. No need to store the sizes when using vectors. Finally, copy the ship vector into each player, using move, in case there is some temporary being passed around.
#include <vector>
struct point {
int x = 0;
int y = 0;
};
struct ship {
int size = 0;
point end_coords[2];
};
struct player {
std::vector<std::vector<int>> map;
std::vector<ship> ships;
player(int map_size, std::vector<ship> ships_) :
map(map_size, std::vector<int>(map_size)),
ships(std::move(ships_)) {}
};
int main()
{
std::vector<ship> ships;
for (int i = 0; i < 5; ++i) {
ships.push_back({ i + 1 });
}
player player1(5, ships);
player player2(5, ships);
return 0;
}
Here I'm not using std::vector (but it's not a good idea). Notice how we can initialize a vector int to 0, and that you don't need a pointer to pointer. With a couple of operators it's much easier to manage the map. As an example I'm printing your map. Suggestion: don't use i and j. Use r and c for rows and columns, to avoid exchanging their meaning by accident.
#include <iostream>
struct point {
int x = 0;
int y = 0;
};
struct ship {
int size = 0;
point end_coords[2];
};
// use _ after names, to make clear that these are members
struct player {
int *map_; // owned
int map_size_;
ship *ships_; // owned
int ships_count_;
// player constructor copies the ships vector
player(int map_size, const ship *ships, int ships_count) :
map_size_{ map_size }, ships_count_{ ships_count }
{
map_ = new int[map_size * map_size]{}; // notice the initializer here to default zero
ships_ = new ship[ships_count];
}
// Rule of 4: if you own memory, you need all these 4 methods. Or delete them.
player(const player&) = delete;
player& operator=(const player&) = delete;
~player() {
delete[] map_; // no setting to nullptr: these will never be accessed anymore
delete[] ships_;
}
int size() const { return map_size_; }
int& operator()(int r, int c) {
return map_[r * map_size_ + c];
}
const int& operator()(int r, int c) const {
return map_[r * map_size_ + c];
}
};
std::ostream& operator<<(std::ostream& os, const player& p) {
auto size = p.size();
for (int r = 0; r < size; ++r) {
for (int c = 0; c < size; ++c) {
if (p(r, c) == 0) {
std::cout << "~";
}
else {
std::cout << p(r, c);
}
}
std::cout << "\n";
}
return os;
}
int main()
{
ship ships[5];
for (int i = 0; i < 5; ++i) {
ships[i].size = i + 1;
}
player player1(5, ships, 5);
player player2(5, ships, 5);
std::cout << "player1:\n" << player1 << "\n";
std::cout << "player2:\n" << player2 << "\n";
return 0;
}