SirVer has proposed merging lp:~widelands-dev/widelands/00_private_inheritance into lp:widelands.
Commit message: - Replace private inheritance with composition everywhere. - Add a lint to forbid private inheritance. - Use std::unique_ptr<> in more places to signify passing or having ownership. - Replace SyncCallback through a std::function<void()>. Requested reviews: Widelands Developers (widelands-dev) For more details, see: https://code.launchpad.net/~widelands-dev/widelands/00_private_inheritance/+merge/326886 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/00_private_inheritance into lp:widelands.
=== added file 'cmake/codecheck/rules/do_not_use_private_inheritance' --- cmake/codecheck/rules/do_not_use_private_inheritance 1970-01-01 00:00:00 +0000 +++ cmake/codecheck/rules/do_not_use_private_inheritance 2017-07-05 19:31:35 +0000 @@ -0,0 +1,21 @@ +#!/usr/bin/python + +""" +strdup isn't available on win32 +""" + +error_msg = 'Do not use strdup/strndup. Use std::string instead.' + +regexp = r'\bstrn?dup\s*\(' + +allowed = [ + 'y = std::string(x)', + + 'y = new char[strlen(x)+1]', + 'std::copy(x, x+strlen(x)+1, y)', +] + +forbidden = [ + 'y = strdup(x)', + 'y = strndup(x)', +] === modified file 'src/game_io/game_loader.cc' --- src/game_io/game_loader.cc 2017-01-25 18:55:59 +0000 +++ src/game_io/game_loader.cc 2017-07-05 19:31:35 +0000 @@ -19,6 +19,8 @@ #include "game_io/game_loader.h" +#include <memory> + #include <boost/bind.hpp> #include <boost/signals2.hpp> @@ -122,8 +124,8 @@ iterate_players_existing_const(p, nr_players, game_, player) { const MessageQueue& messages = player->messages(); for (const auto& temp_message : messages) { - Message* message = temp_message.second; - MessageId message_id = temp_message.first; + const std::unique_ptr<Message>& message = temp_message.second; + const MessageId message_id = temp_message.first; // Renew MapObject connections if (message->serial() > 0) { === modified file 'src/logic/cmd_luacoroutine.cc' --- src/logic/cmd_luacoroutine.cc 2017-01-25 18:55:59 +0000 +++ src/logic/cmd_luacoroutine.cc 2017-07-05 19:31:35 +0000 @@ -19,6 +19,8 @@ #include "logic/cmd_luacoroutine.h" +#include <memory> + #include <boost/format.hpp> #include "base/log.h" @@ -51,11 +53,11 @@ log("%s\n", e.what()); log("Send message to all players and pause game\n"); for (int i = 1; i <= game.map().get_nrplayers(); i++) { - Widelands::Message& msg = *new Widelands::Message( + std::unique_ptr<Message >msg(new Widelands::Message( Message::Type::kGameLogic, game.get_gametime(), "Coroutine", "images/ui_basic/menu_help.png", "Lua Coroutine Failed", - (boost::format("<rt><p font-size=12>%s</p></rt>") % e.what()).str()); - game.player(i).add_message(game, msg, true); + (boost::format("<rt><p font-size=12>%s</p></rt>") % e.what()).str())); + game.player(i).add_message(game, std::move(msg), true); } game.game_controller()->set_desired_speed(0); } === modified file 'src/logic/map_objects/tribes/building.cc' --- src/logic/map_objects/tribes/building.cc 2017-06-26 15:09:21 +0000 +++ src/logic/map_objects/tribes/building.cc 2017-07-05 19:31:35 +0000 @@ -772,13 +772,14 @@ width % img % owner().get_playercolor().hex_value() % UI_FONT_SIZE_MESSAGE % description) .str(); - Message* msg = - new Message(msgtype, game.get_gametime(), title, icon_filename, heading, rt_description, - get_position(), (link_to_building_lifetime ? serial_ : 0)); + std::unique_ptr<Message> msg(new Message(msgtype, game.get_gametime(), title, icon_filename, + heading, rt_description, get_position(), + (link_to_building_lifetime ? serial_ : 0))); - if (throttle_time) - owner().add_message_with_timeout(game, *msg, throttle_time, throttle_radius); - else - owner().add_message(game, *msg); + if (throttle_time) { + owner().add_message_with_timeout(game, std::move(msg), throttle_time, throttle_radius); + } else { + owner().add_message(game, std::move(msg)); + } } } === modified file 'src/logic/map_objects/tribes/ship.cc' --- src/logic/map_objects/tribes/ship.cc 2017-07-01 15:36:36 +0000 +++ src/logic/map_objects/tribes/ship.cc 2017-07-05 19:31:35 +0000 @@ -1068,10 +1068,9 @@ picture % UI_FONT_SIZE_MESSAGE % description) .str(); - Message* msg = new Message(Message::Type::kSeafaring, game.get_gametime(), title, picture, - heading, rt_description, get_position(), serial_); - - get_owner()->add_message(game, *msg); + get_owner()->add_message(game, std::unique_ptr<Message>(new Message( + Message::Type::kSeafaring, game.get_gametime(), title, picture, + heading, rt_description, get_position(), serial_))); } /* === modified file 'src/logic/map_objects/tribes/soldier.cc' --- src/logic/map_objects/tribes/soldier.cc 2017-07-03 19:24:02 +0000 +++ src/logic/map_objects/tribes/soldier.cc 2017-07-05 19:31:35 +0000 @@ -1347,14 +1347,15 @@ descr().descname().c_str()) .str(); owner().add_message( - game, *new Message(Message::Type::kGameLogic, game.get_gametime(), - descr().descname(), "images/ui_basic/menu_help.png", - _("Logic error"), messagetext, get_position(), serial_)); + game, std::unique_ptr<Message>( + new Message(Message::Type::kGameLogic, game.get_gametime(), + descr().descname(), "images/ui_basic/menu_help.png", + _("Logic error"), messagetext, get_position(), serial_))); opponent.owner().add_message( - game, - *new Message(Message::Type::kGameLogic, game.get_gametime(), descr().descname(), - "images/ui_basic/menu_help.png", _("Logic error"), messagetext, - opponent.get_position(), serial_)); + game, std::unique_ptr<Message>(new Message( + Message::Type::kGameLogic, game.get_gametime(), descr().descname(), + "images/ui_basic/menu_help.png", _("Logic error"), messagetext, + opponent.get_position(), serial_))); game.game_controller()->set_desired_speed(0); return pop_task(game); } === modified file 'src/logic/map_objects/tribes/worker.cc' --- src/logic/map_objects/tribes/worker.cc 2017-06-24 08:47:46 +0000 +++ src/logic/map_objects/tribes/worker.cc 2017-07-05 19:31:35 +0000 @@ -899,11 +899,12 @@ // We should add a message to the player's message queue - but only, // if there is not already a similar one in list. - owner().add_message_with_timeout( - game, *new Message(message_type, game.get_gametime(), rdescr->descname(), - ri.descr().representative_image_filename(), rdescr->descname(), - message, position, serial_), - 300000, 8); + owner().add_message_with_timeout(game, + std::unique_ptr<Message>(new Message( + message_type, game.get_gametime(), rdescr->descname(), + ri.descr().representative_image_filename(), + rdescr->descname(), message, position, serial_)), + 300000, 8); } } @@ -1689,10 +1690,11 @@ descr().descname().c_str()) .str(); - owner().add_message(game, *new Message(Message::Type::kGameLogic, game.get_gametime(), - _("Worker"), "images/ui_basic/menu_help.png", - _("Worker got lost!"), message, get_position()), - serial_); + owner().add_message( + game, std::unique_ptr<Message>(new Message(Message::Type::kGameLogic, game.get_gametime(), + _("Worker"), "images/ui_basic/menu_help.png", + _("Worker got lost!"), message, get_position())), + serial_); set_location(nullptr); return pop_task(game); } === modified file 'src/logic/message_queue.h' --- src/logic/message_queue.h 2017-01-25 18:55:59 +0000 +++ src/logic/message_queue.h 2017-07-05 19:31:35 +0000 @@ -22,6 +22,7 @@ #include <cassert> #include <map> +#include <memory> #include "base/macros.h" #include "logic/message.h" @@ -29,7 +30,9 @@ namespace Widelands { -struct MessageQueue : private std::map<MessageId, Message*> { +struct MessageQueue { + using MessageMap = std::map<MessageId, std::unique_ptr<Message>>; + friend class MapPlayersMessagesPacket; MessageQueue() { @@ -39,29 +42,25 @@ } // C++0x: ~MessageQueue() { - while (size()) { - delete begin()->second; - erase(begin()); - } } // Make some selected inherited members public. - MessageQueue::const_iterator begin() const { - return std::map<MessageId, Message*>::begin(); - } - MessageQueue::const_iterator end() const { - return std::map<MessageId, Message*>::end(); - } - size_type count(uint32_t const i) const { + MessageMap::const_iterator begin() const { + return messages_.begin(); + } + MessageMap::const_iterator end() const { + return messages_.end(); + } + size_t count(uint32_t const i) const { assert_counts(); - return std::map<MessageId, Message*>::count(MessageId(i)); + return messages_.count(MessageId(i)); } /// \returns a pointer to the message if it exists, otherwise 0. Message const* operator[](const MessageId& id) const { assert_counts(); - MessageQueue::const_iterator const it = find(MessageId(id)); - return it != end() ? it->second : nullptr; + const auto it = messages_.find(MessageId(id)); + return it != end() ? it->second.get() : nullptr; } /// \returns the number of messages with the given status. @@ -71,20 +70,14 @@ return counts_[static_cast<int>(status)]; } - /// Adds the message. Takes ownership of the message. Assumes that it has - /// been allocated in a separate memory block (not as a component of an - /// array or struct) with operator new, so that it can be deallocated with - /// operator delete. - /// /// \returns the id of the added message. /// /// The loading code calls this function to add messages form the map file. - MessageId add_message(Message& message) { + MessageId add_message(std::unique_ptr<Message> message) { assert_counts(); assert(static_cast<int>(message.status()) < 3); - ++counts_[static_cast<int>(message.status())]; - insert(std::map<MessageId, Message*>::end(), - std::pair<MessageId, Message*>(++current_message_id_, &message)); + ++counts_[static_cast<int>(message->status())]; + messages_[++current_message_id_] = std::move(message); assert_counts(); return current_message_id_; } @@ -93,10 +86,10 @@ void set_message_status(const MessageId& id, Message::Status const status) { assert_counts(); assert(static_cast<int>(status) < 3); - MessageQueue::iterator const it = find(id); - if (it != end()) { + const auto it = messages_.find(id); + if (it != messages_.end()) { Message& message = *it->second; - assert(static_cast<int>(it->second->status()) < 3); + assert(static_cast<int>(message.status()) < 3); assert(counts_[static_cast<int>(message.status())]); --counts_[static_cast<int>(message.status())]; ++counts_[static_cast<int>(message.set_status(status))]; @@ -108,8 +101,8 @@ /// Assumes that a message with the given id exists. void delete_message(const MessageId& id) { assert_counts(); - MessageQueue::iterator const it = find(id); - if (it == end()) { + const auto it = messages_.find(id); + if (it == messages_.end()) { // Messages can be deleted when the linked MapObject is removed. Two delete commands // will be executed, and the message will not be present for the second one. // So we assume here that the message was removed from an earlier delete cmd. @@ -119,8 +112,7 @@ assert(static_cast<int>(message.status()) < 3); assert(counts_[static_cast<int>(message.status())]); --counts_[static_cast<int>(message.status())]; - delete &message; - erase(it); + messages_.erase(it); assert_counts(); } @@ -133,7 +125,7 @@ /// file/savegame but the simulation has not started to run yet. bool is_continuous() const { assert_counts(); - return current_message_id().value() == size(); + return current_message_id().value() == messages_.size(); } private: @@ -147,10 +139,12 @@ counts_[static_cast<int>(Message::Status::kNew)] = 0; counts_[static_cast<int>(Message::Status::kRead)] = 0; counts_[static_cast<int>(Message::Status::kArchived)] = 0; - std::map<MessageId, Message*>::clear(); + messages_.clear(); assert_counts(); } + MessageMap messages_; + /// The id of the most recently added message, or null if none has been /// added yet. MessageId current_message_id_; @@ -160,7 +154,7 @@ uint32_t counts_[3]; void assert_counts() const { - assert(size() == + assert(messages_.size() == counts_[static_cast<int>(Message::Status::kNew)] + counts_[static_cast<int>(Message::Status::kRead)] + counts_[static_cast<int>(Message::Status::kArchived)]); === modified file 'src/logic/player.cc' --- src/logic/player.cc 2017-06-19 06:46:53 +0000 +++ src/logic/player.cc 2017-07-05 19:31:35 +0000 @@ -290,21 +290,22 @@ } } -MessageId Player::add_message(Game& game, Message& message, bool const popup) { - MessageId id = messages().add_message(message); +MessageId Player::add_message(Game& game, std::unique_ptr<Message> new_message, bool const popup) { + MessageId id = messages().add_message(std::move(new_message)); + const Message* message = messages()[id]; // MapObject connection - if (message.serial() > 0) { - MapObject* mo = egbase().objects().get_object(message.serial()); + if (message->serial() > 0) { + MapObject* mo = egbase().objects().get_object(message->serial()); mo->removed.connect(boost::bind(&Player::message_object_removed, this, id)); } // Sound & popup if (InteractivePlayer* const iplayer = game.get_ipl()) { if (&iplayer->player() == this) { - play_message_sound(message.type()); + play_message_sound(message->type()); if (popup) - iplayer->popup_message(id, message); + iplayer->popup_message(id, *message); } } @@ -312,21 +313,20 @@ } MessageId Player::add_message_with_timeout(Game& game, - Message& m, + std::unique_ptr<Message> message, uint32_t const timeout, uint32_t const radius) { const Map& map = game.map(); uint32_t const gametime = game.get_gametime(); - Coords const position = m.position(); - for (auto tmp_message : messages()) { - if (tmp_message.second->type() == m.type() && + Coords const position = message->position(); + for (const auto& tmp_message : messages()) { + if (tmp_message.second->type() == message->type() && gametime < tmp_message.second->sent() + timeout && map.calc_distance(tmp_message.second->position(), position) <= radius) { - delete &m; return MessageId::null(); } } - return add_message(game, m); + return add_message(game, std::move(message)); } void Player::message_object_removed(MessageId message_id) const { === modified file 'src/logic/player.h' --- src/logic/player.h 2017-07-04 11:34:00 +0000 +++ src/logic/player.h 2017-07-05 19:31:35 +0000 @@ -91,16 +91,16 @@ return messages_; } - /// Adds the message to the queue. Takes ownership of the message. Assumes - /// that it has been allocated in a separate memory block (not as a - /// component of an array or struct) with operator new, so that it can be - /// deallocated with operator delete. - MessageId add_message(Game&, Message&, bool popup = false); + /// Adds the message to the queue. + MessageId add_message(Game&, std::unique_ptr<Message> message, bool popup = false); /// Like add_message, but if there has been a message from the same sender /// in the last timeout milliseconds in a radius r around the coordinates /// of m, the message deallocated instead. - MessageId add_message_with_timeout(Game&, Message&, uint32_t timeout, uint32_t radius); + MessageId add_message_with_timeout(Game&, + std::unique_ptr<Message> message, + uint32_t timeout, + uint32_t radius); /// Indicates that the object linked to the message has been removed /// from the game. This implementation deletes the message. === modified file 'src/map_io/map_message_saver.h' --- src/map_io/map_message_saver.h 2017-01-25 18:55:59 +0000 +++ src/map_io/map_message_saver.h 2017-07-05 19:31:35 +0000 @@ -39,19 +39,20 @@ /// CmdDeleteMessage) that refers to a message by id, use this map to /// translate from the id that is stored in the command to the sequence number /// that will be used as the id of the message when the game is loaded. -struct MapMessageSaver : private std::map<MessageId, MessageId> { - MapMessageSaver() : counter(0) { +struct MapMessageSaver { + MapMessageSaver() : counter_(0) { } void add(const MessageId& id) { - assert(find(id) == end()); - insert(std::pair<MessageId, MessageId>(id, ++counter)); + assert(messages_.find(id) == messages_.end()); + messages_.insert(std::pair<MessageId, MessageId>(id, ++counter_)); } MessageId operator[](const MessageId& id) const { - return find(id) != end() ? find(id)->second : MessageId::null(); + return messages_.find(id) != messages_.end() ? messages_.find(id)->second : MessageId::null(); } private: - MessageId counter; + std::map<MessageId, MessageId> messages_; + MessageId counter_; }; } === modified file 'src/map_io/map_players_messages_packet.cc' --- src/map_io/map_players_messages_packet.cc 2017-01-25 18:55:59 +0000 +++ src/map_io/map_players_messages_packet.cc 2017-07-05 19:31:35 +0000 @@ -19,6 +19,8 @@ #include "map_io/map_players_messages_packet.h" +#include <memory> + #include <boost/format.hpp> #include "logic/game_data_error.h" @@ -60,7 +62,8 @@ MessageQueue& messages = player->messages(); { - MessageQueue::const_iterator const begin = messages.begin(); + // NOCOM(#sirver): weird design + const auto begin = messages.begin(); if (begin != messages.end()) { log("ERROR: The message queue for player %u contains a message " "before any messages have been loaded into it. This is a bug " @@ -118,17 +121,17 @@ // Compatibility code needed for map loading. if (packet_version == 1) { const std::string name = s->get_name(); - messages.add_message(*new Message( + messages.add_message(std::unique_ptr<Message>(new Message( static_cast<Message::Type>(s->get_natural("type")), sent, name, "images/wui/fieldaction/menu_build_flag.png", name, s->get_safe_string("body"), - get_coords("position", extent, Coords::null(), s), serial, status)); + get_coords("position", extent, Coords::null(), s), serial, status))); } else { - messages.add_message(*new Message( + messages.add_message(std::unique_ptr<Message>(new Message( static_cast<Message::Type>(s->get_natural("type")), sent, s->get_name(), s->get_safe_string("icon"), s->get_safe_string("heading"), s->get_safe_string("body"), get_coords("position", extent, Coords::null(), s), - serial, status)); + serial, status))); } previous_message_sent = sent; } catch (const WException& e) { === modified file 'src/network/gameclient.cc' --- src/network/gameclient.cc 2017-07-01 08:22:54 +0000 +++ src/network/gameclient.cc 2017-07-05 19:31:35 +0000 @@ -504,7 +504,7 @@ d->lasttimestamp_realtime = SDL_GetTicks(); } -void GameClient::syncreport() { +void GameClient::sync_report_callback() { assert(d->net != nullptr); if (d->net->is_connected()) { SendPacket s; @@ -815,7 +815,7 @@ throw DisconnectException("SYNCREQUEST_WO_GAME"); int32_t const time = packet.signed_32(); d->time.receive(time); - d->game->enqueue_command(new CmdNetCheckSync(time, this)); + d->game->enqueue_command(new CmdNetCheckSync(time, [this] { sync_report_callback(); })); break; } === modified file 'src/network/gameclient.h' --- src/network/gameclient.h 2017-07-01 08:22:54 +0000 +++ src/network/gameclient.h 2017-07-05 19:31:35 +0000 @@ -36,10 +36,7 @@ * This includes running the game setup screen and the actual game after * launch, as well as dealing with the actual network protocol. */ -struct GameClient : public GameController, - public GameSettingsProvider, - private SyncCallback, - public ChatProvider { +struct GameClient : public GameController, public GameSettingsProvider, public ChatProvider { GameClient(const std::pair<NetAddress, NetAddress>& host, const std::string& playername, bool internet = false); @@ -110,7 +107,7 @@ return path + "~backup"; } - void syncreport() override; + void sync_report_callback(); void handle_packet(RecvPacket&); void handle_network(); === modified file 'src/network/gamehost.cc' --- src/network/gamehost.cc 2017-06-25 21:55:39 +0000 +++ src/network/gamehost.cc 2017-07-05 19:31:35 +0000 @@ -1875,7 +1875,8 @@ s.signed_32(d->syncreport_time); broadcast(s); - d->game->enqueue_command(new CmdNetCheckSync(d->syncreport_time, this)); + d->game->enqueue_command( + new CmdNetCheckSync(d->syncreport_time, [this] { sync_report_callback(); })); committed_network_time(d->syncreport_time); } @@ -1923,7 +1924,7 @@ } } -void GameHost::syncreport() { +void GameHost::sync_report_callback() { assert(d->game->get_gametime() == static_cast<uint32_t>(d->syncreport_time)); d->syncreport = d->game->get_sync_hash(); === modified file 'src/network/gamehost.h' --- src/network/gamehost.h 2017-06-24 11:18:12 +0000 +++ src/network/gamehost.h 2017-07-05 19:31:35 +0000 @@ -38,7 +38,7 @@ * This includes running the game setup screen and the actual game after * launch, as well as dealing with the actual network protocol. */ -struct GameHost : public GameController, private SyncCallback { +struct GameHost : public GameController { GameHost(const std::string& playername, bool internet = false); virtual ~GameHost(); @@ -119,7 +119,7 @@ const std::string& c = ""); void request_sync_reports(); void check_sync_reports(); - void syncreport() override; + void sync_report_callback(); void clear_computer_players(); void init_computer_player(Widelands::PlayerNumber p); === modified file 'src/network/network.cc' --- src/network/network.cc 2017-06-26 21:33:46 +0000 +++ src/network/network.cc 2017-07-05 19:31:35 +0000 @@ -77,12 +77,12 @@ return port != 0 && !ip.is_unspecified(); } -CmdNetCheckSync::CmdNetCheckSync(uint32_t const dt, SyncCallback* const cb) +CmdNetCheckSync::CmdNetCheckSync(uint32_t const dt, SyncReportCallback cb) : Command(dt), callback_(cb) { } void CmdNetCheckSync::execute(Widelands::Game&) { - callback_->syncreport(); + callback_(); } NetworkTime::NetworkTime() { === modified file 'src/network/network.h' --- src/network/network.h 2017-06-10 16:36:29 +0000 +++ src/network/network.h 2017-07-05 19:31:35 +0000 @@ -21,6 +21,7 @@ #define WL_NETWORK_NETWORK_H #include <exception> +#include <functional> #include <string> #include <vector> @@ -90,18 +91,14 @@ uint16_t port; }; -struct SyncCallback { - virtual ~SyncCallback() { - } - virtual void syncreport() = 0; -}; +using SyncReportCallback = std::function<void()>; /** * This non-gamelogic command is used by \ref GameHost and \ref GameClient * to schedule taking a synchronization hash. */ struct CmdNetCheckSync : public Widelands::Command { - CmdNetCheckSync(uint32_t dt, SyncCallback*); + CmdNetCheckSync(uint32_t dt, SyncReportCallback); void execute(Widelands::Game&) override; @@ -110,7 +107,7 @@ } private: - SyncCallback* callback_; + SyncReportCallback callback_; }; /** === modified file 'src/scripting/lua_game.cc' --- src/scripting/lua_game.cc 2017-06-23 17:23:04 +0000 +++ src/scripting/lua_game.cc 2017-07-05 19:31:35 +0000 @@ -375,10 +375,10 @@ } } - MessageId const message = - plr.add_message(game, *new Message(Message::Type::kScenario, game.get_gametime(), title, icon, - heading, body, c, 0, st), - popup); + MessageId const message = plr.add_message( + game, std::unique_ptr<Message>(new Message(Message::Type::kScenario, game.get_gametime(), + title, icon, heading, body, c, 0, st)), + popup); return to_lua<LuaMessage>(L, new LuaMessage(player_number(), message)); }
_______________________________________________ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp