From d85348b1d1b9aa7099255eda7c1058f9749d1003 Mon Sep 17 00:00:00 2001 From: Michael Lutz Date: Sun, 28 Nov 2021 17:37:04 +0100 Subject: [PATCH] Codechange: Template the command callback function type to allow unpacked arguments. --- src/command_func.h | 58 ++++++++---- src/network/network_client.cpp | 2 +- src/network/network_command.cpp | 156 ++++++++++++++++++++++---------- src/network/network_gui.cpp | 2 +- src/network/network_server.cpp | 2 +- src/order_backup.cpp | 2 +- src/settings.cpp | 2 +- 7 files changed, 153 insertions(+), 71 deletions(-) diff --git a/src/command_func.h b/src/command_func.h index 2a1c3dc37b..1793812a22 100644 --- a/src/command_func.h +++ b/src/command_func.h @@ -78,6 +78,20 @@ private: static int _counter; }; +#if defined(__GNUC__) && !defined(__clang__) +/* + * We cast specialized function pointers to a generic one, but don't use the + * converted value to call the function, which is safe, except that GCC + * helpfully thinks it is not. + * + * "Any pointer to function can be converted to a pointer to a different function type. + * Calling the function through a pointer to a different function type is undefined, + * but converting such pointer back to pointer to the original function type yields + * the pointer to the original function." */ +# pragma GCC diagnostic push +# pragma GCC diagnostic ignored "-Wcast-function-type" +# define SILENCE_GCC_FUNCTION_POINTER_CAST +#endif template struct CommandHelper; @@ -148,18 +162,19 @@ public: * @param err_message Message prefix to show on error * @param args Parameters for the command */ - static inline bool Post(StringID err_message, Targs... args) { return Post(err_message, nullptr, std::forward(args)...); } + static inline bool Post(StringID err_message, Targs... args) { return Post(err_message, nullptr, std::forward(args)...); } /** * Shortcut for the long Post when not using an error message. * @param callback A callback function to call after the command is finished * @param args Parameters for the command */ - static inline bool Post(CommandCallback *callback, Targs... args) { return Post((StringID)0, callback, std::forward(args)...); } + template + static inline bool Post(Tcallback *callback, Targs... args) { return Post((StringID)0, callback, std::forward(args)...); } /** * Shortcut for the long Post when not using a callback or an error message. * @param args Parameters for the command */ - static inline bool Post(Targs... args) { return Post((StringID)0, nullptr, std::forward(args)...); } + static inline bool Post(Targs... args) { return Post((StringID)0, nullptr, std::forward(args)...); } /** * Top-level network safe command execution for the current company. @@ -171,7 +186,8 @@ public: * @param args Parameters for the command * @return \c true if the command succeeded, else \c false. */ - static bool Post(StringID err_message, CommandCallback *callback, Targs... args) + template + static bool Post(StringID err_message, Tcallback *callback, Targs... args) { return InternalPost(err_message, callback, true, false, std::forward_as_tuple(args...)); } @@ -185,7 +201,8 @@ public: * @param args Parameters for the command * @return \c true if the command succeeded, else \c false. */ - static bool PostFromNet(StringID err_message, CommandCallback *callback, bool my_cmd, TileIndex location, std::tuple args) + template + static bool PostFromNet(StringID err_message, Tcallback *callback, bool my_cmd, TileIndex location, std::tuple args) { return InternalPost(err_message, callback, my_cmd, true, location, std::move(args)); } @@ -194,11 +211,10 @@ public: * Prepare a command to be send over the network * @param cmd The command to execute (a CMD_* value) * @param err_message Message prefix to show on error - * @param callback A callback function to call after the command is finished * @param company The company that wants to send the command * @param args Parameters for the command */ - static void SendNet(StringID err_message, CommandCallback *callback, CompanyID company, Targs... args) + static void SendNet(StringID err_message, CompanyID company, Targs... args) { auto args_tuple = std::forward_as_tuple(args...); @@ -207,7 +223,7 @@ public: tile = std::get<0>(args_tuple); } - ::NetworkSendCommand(Tcmd, err_message, callback, _current_company, tile, EndianBufferWriter::FromValue(args_tuple)); + ::NetworkSendCommand(Tcmd, err_message, nullptr, _current_company, tile, EndianBufferWriter::FromValue(args_tuple)); } /** @@ -220,9 +236,10 @@ public: * @param args Parameters for the command * @return the command cost of this function. */ - static CommandCost Unsafe(StringID err_message, CommandCallback *callback, bool my_cmd, bool estimate_only, TileIndex location, std::tuple args) + template + static CommandCost Unsafe(StringID err_message, Tcallback *callback, bool my_cmd, bool estimate_only, TileIndex location, std::tuple args) { - return Execute(err_message, callback, my_cmd, estimate_only, false, location, std::move(args)); + return Execute(err_message, reinterpret_cast(callback), my_cmd, estimate_only, false, location, std::move(args)); } protected: @@ -242,7 +259,8 @@ protected: ((SetClientIdHelper(std::get(values))), ...); } - static bool InternalPost(StringID err_message, CommandCallback *callback, bool my_cmd, bool network_command, std::tuple args) + template + static bool InternalPost(StringID err_message, Tcallback *callback, bool my_cmd, bool network_command, std::tuple args) { /* Where to show the message? */ TileIndex tile{}; @@ -253,7 +271,8 @@ protected: return InternalPost(err_message, callback, my_cmd, network_command, tile, std::move(args)); } - static bool InternalPost(StringID err_message, CommandCallback *callback, bool my_cmd, bool network_command, TileIndex tile, std::tuple args) + template + static bool InternalPost(StringID err_message, Tcallback *callback, bool my_cmd, bool network_command, TileIndex tile, std::tuple args) { auto [err, estimate_only, only_sending] = InternalPostBefore(Tcmd, GetCommandFlags(), tile, err_message, network_command); if (err) return false; @@ -261,7 +280,7 @@ protected: /* Only set client IDs when the command does not come from the network. */ if (!network_command && GetCommandFlags() & CMD_CLIENT_ID) SetClientIds(args, std::index_sequence_for{}); - CommandCost res = Execute(err_message, callback, my_cmd, estimate_only, network_command, tile, args); + CommandCost res = Execute(err_message, reinterpret_cast(callback), my_cmd, estimate_only, network_command, tile, args); InternalPostResult(res, tile, estimate_only, only_sending, err_message, my_cmd); if (!estimate_only && !only_sending && callback != nullptr) { @@ -360,20 +379,21 @@ struct CommandHelper : Com * @param location Tile location for user feedback. * @param args Parameters for the command */ - static inline bool Post(StringID err_message, TileIndex location, Targs... args) { return Post(err_message, nullptr, location, std::forward(args)...); } + static inline bool Post(StringID err_message, TileIndex location, Targs... args) { return Post(err_message, nullptr, location, std::forward(args)...); } /** * Shortcut for Post when not using a callback. * @param callback A callback function to call after the command is finished * @param location Tile location for user feedback. * @param args Parameters for the command */ - static inline bool Post(CommandCallback *callback, TileIndex location, Targs... args) { return Post((StringID)0, callback, location, std::forward(args)...); } + template + static inline bool Post(Tcallback *callback, TileIndex location, Targs... args) { return Post((StringID)0, callback, location, std::forward(args)...); } /** * Shortcut for Post when not using a callback or an error message. * @param location Tile location for user feedback. * @param args Parameters for the command* */ - static inline bool Post(TileIndex location, Targs... args) { return Post((StringID)0, nullptr, location, std::forward(args)...); } + static inline bool Post(TileIndex location, Targs... args) { return Post((StringID)0, nullptr, location, std::forward(args)...); } /** * Post variant that takes a TileIndex (for error window location and text effects) for @@ -384,12 +404,16 @@ struct CommandHelper : Com * @param args Parameters for the command * @return \c true if the command succeeded, else \c false. */ - static inline bool Post(StringID err_message, CommandCallback *callback, TileIndex location, Targs... args) + template + static inline bool Post(StringID err_message, Tcallback *callback, TileIndex location, Targs... args) { return CommandHelper::InternalPost(err_message, callback, true, false, location, std::forward_as_tuple(args...)); } }; +#ifdef SILENCE_GCC_FUNCTION_POINTER_CAST +# pragma GCC diagnostic pop +#endif template using Command = CommandHelper::ProcType, std::is_same_v::Args>>>; diff --git a/src/network/network_client.cpp b/src/network/network_client.cpp index 1868f5e603..fea018a1c9 100644 --- a/src/network/network_client.cpp +++ b/src/network/network_client.cpp @@ -840,7 +840,7 @@ NetworkRecvStatus ClientNetworkGameSocketHandler::Receive_SERVER_MAP_DONE(Packet * the server will give us a client-id and let us in */ _network_join_status = NETWORK_JOIN_STATUS_REGISTERING; ShowJoinStatusWindow(); - Command::SendNet(STR_NULL, nullptr, _local_company, CCA_NEW, INVALID_COMPANY, CRR_NONE, INVALID_CLIENT_ID); + Command::SendNet(STR_NULL, _local_company, CCA_NEW, INVALID_COMPANY, CRR_NONE, INVALID_CLIENT_ID); } } else { /* take control over an existing company */ diff --git a/src/network/network_command.cpp b/src/network/network_command.cpp index 58de73b198..28300310bd 100644 --- a/src/network/network_command.cpp +++ b/src/network/network_command.cpp @@ -53,55 +53,95 @@ #include "../safeguards.h" -/** Table with all the callbacks we'll use for conversion*/ -static CommandCallback * const _callback_table[] = { - /* 0x00 */ nullptr, - /* 0x01 */ CcBuildPrimaryVehicle, - /* 0x02 */ CcBuildAirport, - /* 0x03 */ CcBuildBridge, - /* 0x04 */ CcPlaySound_CONSTRUCTION_WATER, - /* 0x05 */ CcBuildDocks, - /* 0x06 */ CcFoundTown, - /* 0x07 */ CcBuildRoadTunnel, - /* 0x08 */ CcBuildRailTunnel, - /* 0x09 */ CcBuildWagon, - /* 0x0A */ CcRoadDepot, - /* 0x0B */ CcRailDepot, - /* 0x0C */ CcPlaceSign, - /* 0x0D */ CcPlaySound_EXPLOSION, - /* 0x0E */ CcPlaySound_CONSTRUCTION_OTHER, - /* 0x0F */ CcPlaySound_CONSTRUCTION_RAIL, - /* 0x10 */ CcStation, - /* 0x11 */ CcTerraform, - /* 0x12 */ CcAI, - /* 0x13 */ CcCloneVehicle, - /* 0x14 */ nullptr, - /* 0x15 */ CcCreateGroup, - /* 0x16 */ CcFoundRandomTown, - /* 0x17 */ CcRoadStop, - /* 0x18 */ CcBuildIndustry, - /* 0x19 */ CcStartStopVehicle, - /* 0x1A */ CcGame, - /* 0x1B */ CcAddVehicleNewGroup, -}; +/** Typed list of all possible callbacks. */ +static constexpr auto _callback_tuple = std::make_tuple( + (CommandCallback *)nullptr, // Make sure this is actually a pointer-to-function. + &CcBuildPrimaryVehicle, + &CcBuildAirport, + &CcBuildBridge, + &CcPlaySound_CONSTRUCTION_WATER, + &CcBuildDocks, + &CcFoundTown, + &CcBuildRoadTunnel, + &CcBuildRailTunnel, + &CcBuildWagon, + &CcRoadDepot, + &CcRailDepot, + &CcPlaceSign, + &CcPlaySound_EXPLOSION, + &CcPlaySound_CONSTRUCTION_OTHER, + &CcPlaySound_CONSTRUCTION_RAIL, + &CcStation, + &CcTerraform, + &CcAI, + &CcCloneVehicle, + &CcCreateGroup, + &CcFoundRandomTown, + &CcRoadStop, + &CcBuildIndustry, + &CcStartStopVehicle, + &CcGame, + &CcAddVehicleNewGroup +); + +#ifdef SILENCE_GCC_FUNCTION_POINTER_CAST +/* + * We cast specialized function pointers to a generic one, but don't use the + * converted value to call the function, which is safe, except that GCC + * helpfully thinks it is not. + * + * "Any pointer to function can be converted to a pointer to a different function type. + * Calling the function through a pointer to a different function type is undefined, + * but converting such pointer back to pointer to the original function type yields + * the pointer to the original function." */ +# pragma GCC diagnostic push +# pragma GCC diagnostic ignored "-Wcast-function-type" +#endif + +/* Helpers to generate the callback table from the callback list. */ + +inline constexpr size_t _callback_tuple_size = std::tuple_size_v; + +template +inline auto MakeCallbackTable(std::index_sequence) noexcept +{ + return std::array{{ reinterpret_cast(reinterpret_cast(std::get(_callback_tuple)))... }}; // MingW64 fails linking when casting a pointer to its own type. To work around, cast it to some other type first. +} + +/** Type-erased table of callbacks. */ +static auto _callback_table = MakeCallbackTable(std::make_index_sequence<_callback_tuple_size>{}); + /* Helpers to generate the command dispatch table from the command traits. */ template static CommandDataBuffer SanitizeCmdStrings(const CommandDataBuffer &data); -template static void UnpackNetworkCommand(const CommandPacket *cp); +template static void UnpackNetworkCommand(const CommandPacket *cp); template static void NetworkReplaceCommandClientId(CommandPacket &cp, ClientID client_id); +using UnpackNetworkCommandProc = void (*)(const CommandPacket *); +using UnpackDispatchT = std::array; struct CommandDispatch { CommandDataBuffer(*Sanitize)(const CommandDataBuffer &); void (*ReplaceClientId)(CommandPacket &, ClientID); - void (*Unpack)(const CommandPacket *); + UnpackDispatchT Unpack; }; -template -inline constexpr auto MakeDispatchTable(std::integer_sequence) noexcept +template +constexpr UnpackDispatchT MakeUnpackNetworkCommand(std::index_sequence) noexcept { - return std::array{{ { &SanitizeCmdStrings(i)>, &NetworkReplaceCommandClientId(i)>, &UnpackNetworkCommand(i)> }... }}; + return UnpackDispatchT{{ {&UnpackNetworkCommand}... }}; } -static constexpr auto _cmd_dispatch = MakeDispatchTable(std::make_integer_sequence, CMD_END>{}); + +template +inline constexpr auto MakeDispatchTable(std::integer_sequence, std::index_sequence) noexcept +{ + return std::array{{ { &SanitizeCmdStrings(i)>, &NetworkReplaceCommandClientId(i)>, MakeUnpackNetworkCommand(i)>(std::make_index_sequence<_callback_tuple_size>{}) }... }}; +} +/** Command dispatch table. */ +static constexpr auto _cmd_dispatch = MakeDispatchTable(std::make_integer_sequence, CMD_END>{}, std::make_index_sequence<_callback_tuple_size>{}); + +#ifdef SILENCE_GCC_FUNCTION_POINTER_CAST +# pragma GCC diagnostic pop +#endif /** @@ -179,6 +219,20 @@ static CommandQueue _local_wait_queue; static CommandQueue _local_execution_queue; +/** + * Find the callback index of a callback pointer. + * @param callback Address of callback to search for. + * @return Callback index or std::numeric_limits::max() if the function wasn't found in the callback list. + */ +static size_t FindCallbackIndex(CommandCallback *callback) +{ + if (auto it = std::find(std::cbegin(_callback_table), std::cend(_callback_table), callback); it != std::cend(_callback_table)) { + return static_cast(std::distance(std::cbegin(_callback_table), it)); + } + + return std::numeric_limits::max(); +} + /** * Prepare a DoCommand to be send over the network * @param cmd The command to execute (a CMD_* value) @@ -259,7 +313,9 @@ void NetworkExecuteLocalCommandQueue() /* We can execute this command */ _current_company = cp->company; - _cmd_dispatch[cp->cmd].Unpack(cp); + size_t cb_index = FindCallbackIndex(cp->callback); + assert(cb_index < _callback_tuple_size); + _cmd_dispatch[cp->cmd].Unpack[cb_index](cp); queue.Pop(); delete cp; @@ -354,7 +410,7 @@ const char *NetworkGameSocketHandler::ReceiveCommand(Packet *p, CommandPacket *c cp->data = _cmd_dispatch[cp->cmd].Sanitize(p->Recv_buffer()); byte callback = p->Recv_uint8(); - if (callback >= lengthof(_callback_table)) return "invalid callback"; + if (callback >= _callback_table.size()) return "invalid callback"; cp->callback = _callback_table[callback]; return nullptr; @@ -373,16 +429,12 @@ void NetworkGameSocketHandler::SendCommand(Packet *p, const CommandPacket *cp) p->Send_uint32(cp->tile); p->Send_buffer(cp->data); - byte callback = 0; - while (callback < lengthof(_callback_table) && _callback_table[callback] != cp->callback) { - callback++; - } - - if (callback == lengthof(_callback_table)) { + size_t callback = FindCallbackIndex(cp->callback); + if (callback > UINT8_MAX) { Debug(net, 0, "Unknown callback for command; no callback sent (command: {})", cp->cmd); callback = 0; // _callback_table[0] == nullptr } - p->Send_uint8 (callback); + p->Send_uint8 ((uint8)callback); } /** Helper to process a single ClientID argument. */ @@ -455,9 +507,15 @@ CommandDataBuffer SanitizeCmdStrings(const CommandDataBuffer &data) return EndianBufferWriter::FromValue(args); } -template -void UnpackNetworkCommand(const CommandPacket *cp) +/** + * Unpack a generic command packet into its actual typed components. + * @tparam Tcmd Command type to be unpacked. + * @tparam Tcb Index into the callback list. + * @param cp Command packet to unpack. + */ +template +void UnpackNetworkCommand(const CommandPacket* cp) { auto args = EndianBufferReader::ToValue::Args>(cp->data); - Command::PostFromNet(cp->err_msg, cp->callback, cp->my_cmd, cp->tile, args); + Command::PostFromNet(cp->err_msg, std::get(_callback_tuple), cp->my_cmd, cp->tile, args); } diff --git a/src/network/network_gui.cpp b/src/network/network_gui.cpp index 0c7e361dd4..007dddb6d0 100644 --- a/src/network/network_gui.cpp +++ b/src/network/network_gui.cpp @@ -1538,7 +1538,7 @@ private: if (_network_server) { Command::Post(CCA_NEW, INVALID_COMPANY, CRR_NONE, _network_own_client_id); } else { - Command::SendNet(STR_NULL, nullptr, _local_company, CCA_NEW, INVALID_COMPANY, CRR_NONE, INVALID_CLIENT_ID); + Command::SendNet(STR_NULL, _local_company, CCA_NEW, INVALID_COMPANY, CRR_NONE, INVALID_CLIENT_ID); } } diff --git a/src/network/network_server.cpp b/src/network/network_server.cpp index 36ab3840a4..d3cbdf9e3a 100644 --- a/src/network/network_server.cpp +++ b/src/network/network_server.cpp @@ -2081,7 +2081,7 @@ void NetworkServerNewCompany(const Company *c, NetworkClientInfo *ci) /* ci is nullptr when replaying, or for AIs. In neither case there is a client. */ ci->client_playas = c->index; NetworkUpdateClientInfo(ci->client_id); - Command::SendNet(STR_NULL, nullptr, c->index, ci->client_name); + Command::SendNet(STR_NULL, c->index, ci->client_name); } /* Announce new company on network. */ diff --git a/src/order_backup.cpp b/src/order_backup.cpp index 46a55991da..5e634d6958 100644 --- a/src/order_backup.cpp +++ b/src/order_backup.cpp @@ -198,7 +198,7 @@ CommandCost CmdClearOrderBackup(DoCommandFlag flags, TileIndex tile, ClientID us /* We need to circumvent the "prevention" from this command being executed * while the game is paused, so use the internal method. Nor do we want * this command to get its cost estimated when shift is pressed. */ - Command::Unsafe(STR_NULL, nullptr, true, false, ob->tile, CommandTraits::Args{ ob->tile, static_cast(user) }); + Command::Unsafe(STR_NULL, nullptr, true, false, ob->tile, CommandTraits::Args{ ob->tile, static_cast(user) }); } else { /* The command came from the game logic, i.e. the clearing of a tile. * In that case we have no need to actually sync this, just do it. */ diff --git a/src/settings.cpp b/src/settings.cpp index 25eeb964a5..f619760284 100644 --- a/src/settings.cpp +++ b/src/settings.cpp @@ -1600,7 +1600,7 @@ void SyncCompanySettings() const SettingDesc *sd = GetSettingDesc(desc); uint32 old_value = (uint32)sd->AsIntSetting()->Read(new_object); uint32 new_value = (uint32)sd->AsIntSetting()->Read(old_object); - if (old_value != new_value) Command::SendNet(STR_NULL, nullptr, _local_company, sd->GetName(), new_value); + if (old_value != new_value) Command::SendNet(STR_NULL, _local_company, sd->GetName(), new_value); } }