From e105412d678b47fd1388c178334874cb01f44380 Mon Sep 17 00:00:00 2001 From: mrkubax10 Date: Mon, 21 Aug 2023 13:01:40 +0200 Subject: [PATCH] Fix memory leaks and code quality in autotile code (#2591) Fixes various code quality related issues in autotile code. Also fixes all memory leaks found by AddressSanitizer. Game doesn't use heap allocation anymore where it's not neccessary and uses smart pointers where possible/required. --- src/supertux/autotile.cpp | 67 ++++++++++---------------------- src/supertux/autotile.hpp | 40 +++++++------------ src/supertux/autotile_parser.cpp | 30 +++++++------- src/supertux/autotile_parser.hpp | 12 +++--- src/supertux/tile_set.cpp | 10 +---- src/supertux/tile_set.hpp | 6 +-- src/supertux/tile_set_parser.cpp | 4 +- 7 files changed, 60 insertions(+), 109 deletions(-) diff --git a/src/supertux/autotile.cpp b/src/supertux/autotile.cpp index be546aea1d5..40ef34d01a5 100644 --- a/src/supertux/autotile.cpp +++ b/src/supertux/autotile.cpp @@ -14,11 +14,11 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -#include - #include "supertux/autotile.hpp" -//#include "supertux/autotile_parser.hpp" +#include + +#include "util/log.hpp" // AutotileMask. @@ -34,17 +34,11 @@ AutotileMask::matches(uint8_t mask, bool center) const return mask == m_mask && center == m_center; } -uint8_t -AutotileMask::get_mask() const -{ - return m_mask; -} - // Autotile. -Autotile::Autotile(uint32_t tile_id, std::vector> alt_tiles, std::vector masks, bool solid) : +Autotile::Autotile(uint32_t tile_id, const std::vector>& alt_tiles, const std::vector& masks, bool solid) : m_tile_id(tile_id), - m_alt_tiles(std::move(alt_tiles)), + m_alt_tiles(alt_tiles), m_masks(std::move(masks)), m_solid(solid) { @@ -55,7 +49,7 @@ Autotile::matches(uint8_t num_mask, bool center) const { for (auto& l_mask : m_masks) { - if (l_mask->matches(num_mask, center)) + if (l_mask.matches(num_mask, center)) { return true; } @@ -63,12 +57,6 @@ Autotile::matches(uint8_t num_mask, bool center) const return false; } -uint32_t -Autotile::get_tile_id() const -{ - return m_tile_id; -} - uint32_t Autotile::pick_tile(int x, int y) const { @@ -116,35 +104,28 @@ uint8_t Autotile::get_first_mask() const { if (!m_masks.empty()) - return m_masks[0]->get_mask(); + return m_masks[0].get_mask(); return 0; } -std::vector> -Autotile::get_all_tile_ids() const -{ - return m_alt_tiles; -} - -bool -Autotile::is_solid() const -{ - return m_solid; -} - - // AutotileSet. -std::vector* AutotileSet::m_autotilesets = new std::vector(); +std::vector> AutotileSet::m_autotilesets; -AutotileSet::AutotileSet(std::vector tiles, uint32_t default_tile, std::string name, bool corner) : - m_autotiles(std::move(tiles)), +AutotileSet::AutotileSet(const std::vector& tiles, uint32_t default_tile, const std::string& name, bool corner) : + m_autotiles(tiles), m_default(default_tile), - m_name(std::move(name)), + m_name(name), m_corner(corner) { } +AutotileSet::~AutotileSet() +{ + for (Autotile* autotile : m_autotiles) + delete autotile; +} + /* AutotileSet* AutotileSet::get_tileset_from_tile(uint32_t tile_id) @@ -210,7 +191,7 @@ AutotileSet::get_autotile(uint32_t tile_id, if (top_left) num_mask = static_cast(num_mask + 0x80); } - for (auto& autotile : m_autotiles) + for (auto* autotile : m_autotiles) { if (autotile->matches(num_mask, center)) { @@ -221,12 +202,6 @@ AutotileSet::get_autotile(uint32_t tile_id, return center ? get_default_tile() : 0; } -uint32_t -AutotileSet::get_default_tile() const -{ - return m_default; -} - bool AutotileSet::is_member(uint32_t tile_id) const { @@ -258,7 +233,7 @@ AutotileSet::is_solid(uint32_t tile_id) const if (!is_member(tile_id)) return false; - for (auto& tile : m_autotiles) + for (auto* tile : m_autotiles) { if (tile->get_tile_id() == tile_id) { @@ -286,7 +261,7 @@ AutotileSet::is_solid(uint32_t tile_id) const uint8_t AutotileSet::get_mask_from_tile(uint32_t tile) const { - for (auto& autotile : m_autotiles) + for (auto* autotile : m_autotiles) { if (autotile->is_amongst(tile)) { return autotile->get_first_mask(); @@ -307,7 +282,7 @@ AutotileSet::validate() const uint32_t tile_nonsolid = 0; // Relevant only for non-corner autotiles. uint32_t tile_with_that_mask = 0; // Used to help users debug. - for (auto& autotile : m_autotiles) + for (auto* autotile : m_autotiles) { if (autotile->matches(num_mask, true)) { diff --git a/src/supertux/autotile.hpp b/src/supertux/autotile.hpp index 9ad4dcce3d8..e5951c7716c 100644 --- a/src/supertux/autotile.hpp +++ b/src/supertux/autotile.hpp @@ -17,22 +17,11 @@ #ifndef HEADER_SUPERTUX_SUPERTUX_AUTOTILE_HPP #define HEADER_SUPERTUX_SUPERTUX_AUTOTILE_HPP +#include #include #include #include -#include - -#include "math/rect.hpp" -#include "math/rectf.hpp" -#include "math/size.hpp" -#include "object/path_object.hpp" -#include "object/path_walker.hpp" -#include "squirrel/exposed_object.hpp" -#include "scripting/tilemap.hpp" -#include "supertux/game_object.hpp" -#include "video/color.hpp" -#include "video/flip.hpp" -#include "video/drawing_target.hpp" +#include class AutotileMask final { @@ -41,29 +30,25 @@ class AutotileMask final bool matches(uint8_t mask, bool center) const; - uint8_t get_mask() const; + uint8_t get_mask() const { return m_mask; } private: uint8_t m_mask; bool m_center; // m_center should *always* be the same as the m_solid of the corresponding Autotile - -private: - AutotileMask(const AutotileMask&) = delete; - AutotileMask& operator=(const AutotileMask&) = delete; }; class Autotile final { public: Autotile(uint32_t tile_id, - std::vector> alt_tiles, - std::vector masks, + const std::vector>& alt_tiles, + const std::vector& masks, bool solid); bool matches(uint8_t mask, bool center) const; /** @deprecated Returns the base tile ID. */ - uint32_t get_tile_id() const; + uint32_t get_tile_id() const { return m_tile_id; } /** Picks a tile randomly amongst the possible ones for this autotile. */ uint32_t pick_tile(int x, int y) const; @@ -75,15 +60,15 @@ class Autotile final uint8_t get_first_mask() const; /** Returns all possible tiles for this autotile */ - std::vector> get_all_tile_ids() const; + const std::vector>& get_all_tile_ids() const { return m_alt_tiles; } /** Returns true if the "center" bool of masks are true. All masks of given Autotile must have the same value for their "center" property.*/ - bool is_solid() const; + bool is_solid() const { return m_solid; } private: uint32_t m_tile_id; std::vector> m_alt_tiles; - std::vector m_masks; + std::vector m_masks; bool m_solid; private: @@ -98,7 +83,8 @@ class AutotileSet final //static AutotileSet* get_tileset_from_tile(uint32_t tile_id); public: - AutotileSet(std::vector autotiles, uint32_t default_tile, std::string name, bool corner); + AutotileSet(const std::vector& autotiles, uint32_t default_tile, const std::string& name, bool corner); + ~AutotileSet(); /** Returns the ID of the tile to use, based on the surrounding tiles. * If the autotileset is corner-based, the top, left, right, bottom and @@ -112,7 +98,7 @@ class AutotileSet final ) const; /** Returns the id of the first block in the autotileset. Used for erronous configs. */ - uint32_t get_default_tile() const; + uint32_t get_default_tile() const { return m_default; } /** true if the given tile is present in the autotileset */ bool is_member(uint32_t tile_id) const; @@ -133,7 +119,7 @@ class AutotileSet final void validate() const; public: - static std::vector* m_autotilesets; + static std::vector> m_autotilesets; private: std::vector m_autotiles; diff --git a/src/supertux/autotile_parser.cpp b/src/supertux/autotile_parser.cpp index fae7495b3dc..12baa45b0e5 100644 --- a/src/supertux/autotile_parser.cpp +++ b/src/supertux/autotile_parser.cpp @@ -1,7 +1,5 @@ // SuperTux -// Copyright (C) 2008-2020 A. Semphris , -// Matthias Braun , -// Ingo Ruhnke +// Copyright (C) 2020 A. Semphris // // This program is free software: you can redistribute it and/or modify // it under the terms of the GNU General Public License as published by @@ -29,7 +27,7 @@ #include "util/reader_mapping.hpp" #include "util/file_system.hpp" -AutotileParser::AutotileParser(std::vector* autotilesets, const std::string& filename) : +AutotileParser::AutotileParser(std::vector>& autotilesets, const std::string& filename) : m_autotilesets(autotilesets), m_filename(filename), m_tiles_path() @@ -71,7 +69,7 @@ AutotileParser::parse() void AutotileParser::parse_autotileset(const ReaderMapping& reader, bool corner) { - std::vector* autotiles = new std::vector(); + std::vector autotiles; std::string name = "[unnamed]"; if (!reader.get("name", name)) @@ -91,7 +89,7 @@ AutotileParser::parse_autotileset(const ReaderMapping& reader, bool corner) if (iter.get_key() == "autotile") { ReaderMapping tile_mapping = iter.as_mapping(); - autotiles->push_back(parse_autotile(tile_mapping, corner)); + autotiles.push_back(parse_autotile(tile_mapping, corner)); } else if (iter.get_key() != "name" && iter.get_key() != "default") { @@ -99,20 +97,20 @@ AutotileParser::parse_autotileset(const ReaderMapping& reader, bool corner) } } - AutotileSet* autotileset = new AutotileSet(*autotiles, default_id, name, corner); + std::unique_ptr autotileset = std::make_unique(autotiles, default_id, name, corner); if (g_config->developer_mode) { autotileset->validate(); } - m_autotilesets->push_back(autotileset); + m_autotilesets.push_back(std::move(autotileset)); } Autotile* AutotileParser::parse_autotile(const ReaderMapping& reader, bool corner) { - std::vector autotile_masks; + std::vector autotile_masks; std::vector> alt_ids; uint32_t tile_id; @@ -143,11 +141,11 @@ AutotileParser::parse_autotile(const ReaderMapping& reader, bool corner) if (corner) { - parse_mask_corner(mask, &autotile_masks); + parse_mask_corner(mask, autotile_masks); } else { - parse_mask(mask, &autotile_masks, solid); + parse_mask(mask, autotile_masks, solid); } } else if (iter.get_key() == "alt-id") @@ -177,11 +175,11 @@ AutotileParser::parse_autotile(const ReaderMapping& reader, bool corner) } } - return new Autotile(tile_id, alt_ids, autotile_masks, !!solid); + return new Autotile(tile_id, alt_ids, autotile_masks, solid); } void -AutotileParser::parse_mask(std::string mask, std::vector* autotile_masks, bool solid) +AutotileParser::parse_mask(std::string mask, std::vector& autotile_masks, bool solid) { if (mask.size() != 8) { @@ -224,12 +222,12 @@ AutotileParser::parse_mask(std::string mask, std::vector* autotil for (uint8_t val : masks) { - autotile_masks->push_back(new AutotileMask(val, solid)); + autotile_masks.push_back(AutotileMask(val, solid)); } } void -AutotileParser::parse_mask_corner(std::string mask, std::vector* autotile_masks) +AutotileParser::parse_mask_corner(std::string mask, std::vector& autotile_masks) { if (mask.size() != 4) { @@ -272,7 +270,7 @@ AutotileParser::parse_mask_corner(std::string mask, std::vector* for (uint8_t val : masks) { - autotile_masks->push_back(new AutotileMask(val, true)); + autotile_masks.push_back(AutotileMask(val, true)); } } diff --git a/src/supertux/autotile_parser.hpp b/src/supertux/autotile_parser.hpp index ba607c33b17..7f68c25ff97 100644 --- a/src/supertux/autotile_parser.hpp +++ b/src/supertux/autotile_parser.hpp @@ -1,7 +1,5 @@ // SuperTux -// Copyright (C) 2008-2020 A. Semphris , -// Matthias Braun , -// Ingo Ruhnke +// Copyright (C) 2020 A. Semphris // // This program is free software: you can redistribute it and/or modify // it under the terms of the GNU General Public License as published by @@ -29,20 +27,20 @@ class ReaderMapping; class AutotileParser final { private: - std::vector* m_autotilesets; + std::vector>& m_autotilesets; std::string m_filename; std::string m_tiles_path; public: - AutotileParser(std::vector* autotilesets, const std::string& filename); + AutotileParser(std::vector>& autotilesets, const std::string& filename); void parse(); private: void parse_autotileset(const ReaderMapping& reader, bool corner); Autotile* parse_autotile(const ReaderMapping& reader, bool corner); - void parse_mask(std::string mask, std::vector* autotile_masks, bool solid); - void parse_mask_corner(std::string mask, std::vector* autotile_masks); + void parse_mask(std::string mask, std::vector& autotile_masks, bool solid); + void parse_mask_corner(std::string mask, std::vector& autotile_masks); private: AutotileParser(const AutotileParser&) = delete; diff --git a/src/supertux/tile_set.cpp b/src/supertux/tile_set.cpp index 523935f1868..5919cb9e238 100644 --- a/src/supertux/tile_set.cpp +++ b/src/supertux/tile_set.cpp @@ -53,12 +53,6 @@ TileSet::TileSet() : m_tilegroups() { m_tiles[0] = std::make_unique(); - m_autotilesets = new std::vector(); -} - -TileSet::~TileSet() -{ - delete m_autotilesets; } void @@ -101,11 +95,11 @@ TileSet::get_autotileset_from_tile(uint32_t tile_id) const return nullptr; } - for (auto& ats : *m_autotilesets) + for (auto& ats : m_autotilesets) { if (ats->is_member(tile_id)) { - return ats; + return ats.get(); } } return nullptr; diff --git a/src/supertux/tile_set.hpp b/src/supertux/tile_set.hpp index 91d32ae9eeb..c954ce08956 100644 --- a/src/supertux/tile_set.hpp +++ b/src/supertux/tile_set.hpp @@ -24,12 +24,12 @@ #include "math/fwd.hpp" #include "supertux/autotile.hpp" +#include "supertux/tile.hpp" #include "video/color.hpp" #include "video/surface_ptr.hpp" class Canvas; class DrawingContext; -class Tile; class Tilegroup final { @@ -48,7 +48,7 @@ class TileSet final public: TileSet(); - ~TileSet(); + ~TileSet() = default; void add_tile(int id, std::unique_ptr tile); @@ -74,7 +74,7 @@ class TileSet final public: // Must be public because of tile_set_parser.cpp - std::vector* m_autotilesets; + std::vector> m_autotilesets; // Additional attributes diff --git a/src/supertux/tile_set_parser.cpp b/src/supertux/tile_set_parser.cpp index c53a8b2cb98..74cd090aebd 100644 --- a/src/supertux/tile_set_parser.cpp +++ b/src/supertux/tile_set_parser.cpp @@ -102,9 +102,9 @@ TileSetParser::parse(int32_t start, int32_t end, int32_t offset, bool imported) } else { - AutotileParser* parser = new AutotileParser(m_tileset.m_autotilesets, + AutotileParser parser(m_tileset.m_autotilesets, FileSystem::normalize(m_tiles_path + autotile_filename)); - parser->parse(); + parser.parse(); } } else if (iter.get_key() == "import-tileset")