From 20d83677ebdb6a1c5233ce10e7b1e2c7bb5d6872 Mon Sep 17 00:00:00 2001 From: Peter Nelson Date: Thu, 10 Apr 2025 08:34:59 +0100 Subject: [PATCH] Codechange: Use unique_ptr to manage ContentInfo lifetime. Removes manually managed new/delete. --- src/console_cmds.cpp | 12 ++--- src/network/network_content.cpp | 69 ++++++++++++----------------- src/network/network_content.h | 27 +++++------ src/network/network_content_gui.cpp | 18 +++----- src/newgrf_gui.cpp | 4 +- 5 files changed, 52 insertions(+), 78 deletions(-) diff --git a/src/console_cmds.cpp b/src/console_cmds.cpp index 327cb7eb54..cb091832ff 100644 --- a/src/console_cmds.cpp +++ b/src/console_cmds.cpp @@ -2171,9 +2171,9 @@ DEF_CONSOLE_CMD(ConContent) if (argc <= 2) { /* List selected content */ IConsolePrint(CC_WHITE, "id, type, state, name"); - for (ConstContentIterator iter = _network_content_client.Begin(); iter != _network_content_client.End(); iter++) { - if ((*iter)->state != ContentInfo::SELECTED && (*iter)->state != ContentInfo::AUTOSELECTED) continue; - OutputContentState(**iter); + for (const ContentInfo &ci : _network_content_client.Info()) { + if (ci.state != ContentInfo::SELECTED && ci.state != ContentInfo::AUTOSELECTED) continue; + OutputContentState(ci); } } else if (StrEqualsIgnoreCase(argv[2], "all")) { /* The intention of this function was that you could download @@ -2204,9 +2204,9 @@ DEF_CONSOLE_CMD(ConContent) if (StrEqualsIgnoreCase(argv[1], "state")) { IConsolePrint(CC_WHITE, "id, type, state, name"); - for (ConstContentIterator iter = _network_content_client.Begin(); iter != _network_content_client.End(); iter++) { - if (argc > 2 && !StrContainsIgnoreCase((*iter)->name, argv[2])) continue; - OutputContentState(**iter); + for (const ContentInfo &ci : _network_content_client.Info()) { + if (argc > 2 && !StrContainsIgnoreCase(ci.name, argv[2])) continue; + OutputContentState(ci); } return true; } diff --git a/src/network/network_content.cpp b/src/network/network_content.cpp index eb6bc31db4..d93fc012e7 100644 --- a/src/network/network_content.cpp +++ b/src/network/network_content.cpp @@ -82,7 +82,7 @@ static HasContentProc *GetHasContentProcforContentType(ContentType type) bool ClientNetworkContentSocketHandler::Receive_SERVER_INFO(Packet &p) { - ContentInfo *ci = new ContentInfo(); + auto ci = std::make_unique(); ci->type = (ContentType)p.Recv_uint8(); ci->id = (ContentID)p.Recv_uint32(); ci->filesize = p.Recv_uint32(); @@ -108,7 +108,6 @@ bool ClientNetworkContentSocketHandler::Receive_SERVER_INFO(Packet &p) for (uint i = 0; i < tag_count; i++) ci->tags.push_back(p.Recv_string(NETWORK_CONTENT_TAG_LENGTH)); if (!ci->IsValid()) { - delete ci; this->CloseConnection(); return false; } @@ -129,7 +128,7 @@ bool ClientNetworkContentSocketHandler::Receive_SERVER_INFO(Packet &p) if (ci->state == ContentInfo::UNSELECTED && ci->filesize == 0) ci->state = ContentInfo::DOES_NOT_EXIST; /* Do we already have a stub for this? */ - for (ContentInfo *ici : this->infos) { + for (const auto &ici : this->infos) { if (ici->type == ci->type && ici->unique_id == ci->unique_id && ci->md5sum == ici->md5sum) { /* Preserve the name if possible */ if (ci->name.empty()) ci->name = ici->name; @@ -141,7 +140,6 @@ bool ClientNetworkContentSocketHandler::Receive_SERVER_INFO(Packet &p) * we (just) already preserved. */ *ici = *ci; - delete ci; this->OnReceiveContentInfo(*ici); return true; @@ -150,20 +148,19 @@ bool ClientNetworkContentSocketHandler::Receive_SERVER_INFO(Packet &p) /* Missing content info? Don't list it */ if (ci->filesize == 0) { - delete ci; return true; } - this->infos.push_back(ci); + ContentInfo *info = this->infos.emplace_back(std::move(ci)).get(); /* Incoming data means that we might need to reconsider dependencies */ ConstContentVector parents; - this->ReverseLookupTreeDependency(parents, ci); + this->ReverseLookupTreeDependency(parents, info); for (const ContentInfo *ici : parents) { this->CheckDependencyState(*ici); } - this->OnReceiveContentInfo(*ci); + this->OnReceiveContentInfo(*info); return true; } @@ -257,7 +254,7 @@ void ClientNetworkContentSocketHandler::RequestContentList(ContentVector *cv, bo auto p = std::make_unique(this, send_md5sum ? PACKET_CONTENT_CLIENT_INFO_EXTID_MD5 : PACKET_CONTENT_CLIENT_INFO_EXTID, TCP_MTU); p->Send_uint8((uint8_t)cv->size()); - for (const ContentInfo *ci : *cv) { + for (const auto &ci : *cv) { p->Send_uint8((uint8_t)ci->type); p->Send_uint32(ci->unique_id); if (!send_md5sum) continue; @@ -266,9 +263,9 @@ void ClientNetworkContentSocketHandler::RequestContentList(ContentVector *cv, bo this->SendPacket(std::move(p)); - for (ContentInfo *ci : *cv) { + for (auto &ci : *cv) { bool found = false; - for (ContentInfo *ci2 : this->infos) { + for (const auto &ci2 : this->infos) { if (ci->type == ci2->type && ci->unique_id == ci2->unique_id && (!send_md5sum || ci->md5sum == ci2->md5sum)) { found = true; @@ -276,9 +273,7 @@ void ClientNetworkContentSocketHandler::RequestContentList(ContentVector *cv, bo } } if (!found) { - this->infos.push_back(ci); - } else { - delete ci; + this->infos.push_back(std::move(ci)); } } } @@ -294,7 +289,7 @@ void ClientNetworkContentSocketHandler::DownloadSelectedContent(uint &files, uin bytes = 0; ContentIDList content; - for (const ContentInfo *ci : this->infos) { + for (const auto &ci : this->infos) { if (!ci->IsSelected() || ci->state == ContentInfo::ALREADY_HERE) continue; content.push_back(ci->id); @@ -368,13 +363,13 @@ void ClientNetworkContentSocketHandler::DownloadSelectedContentFallback(const Co * @return a statically allocated buffer with the filename or * nullptr when no filename could be made. */ -static std::string GetFullFilename(const ContentInfo *ci, bool compressed) +static std::string GetFullFilename(const ContentInfo &ci, bool compressed) { - Subdirectory dir = GetContentInfoSubDir(ci->type); + Subdirectory dir = GetContentInfoSubDir(ci.type); if (dir == NO_DIRECTORY) return {}; std::string buf = FioGetDirectory(SP_AUTODOWNLOAD_DIR, dir); - buf += ci->filename; + buf += ci.filename; buf += compressed ? ".tar.gz" : ".tar"; return buf; @@ -385,7 +380,7 @@ static std::string GetFullFilename(const ContentInfo *ci, bool compressed) * @param ci container with filename * @return true if the gunzip completed */ -static bool GunzipFile(const ContentInfo *ci) +static bool GunzipFile(const ContentInfo &ci) { #if defined(WITH_ZLIB) bool ret = true; @@ -462,9 +457,8 @@ static inline ssize_t TransferOutFWrite(std::optional &file, const c bool ClientNetworkContentSocketHandler::Receive_SERVER_CONTENT(Packet &p) { if (!this->cur_file.has_value()) { - delete this->cur_info; /* When we haven't opened a file this must be our first packet with metadata. */ - this->cur_info = new ContentInfo; + this->cur_info = std::make_unique(); this->cur_info->type = (ContentType)p.Recv_uint8(); this->cur_info->id = (ContentID)p.Recv_uint32(); this->cur_info->filesize = p.Recv_uint32(); @@ -504,14 +498,13 @@ bool ClientNetworkContentSocketHandler::Receive_SERVER_CONTENT(Packet &p) bool ClientNetworkContentSocketHandler::BeforeDownload() { if (!this->cur_info->IsValid()) { - delete this->cur_info; - this->cur_info = nullptr; + this->cur_info.reset(); return false; } if (this->cur_info->filesize != 0) { /* The filesize is > 0, so we are going to download it */ - std::string filename = GetFullFilename(this->cur_info, true); + std::string filename = GetFullFilename(*this->cur_info, true); if (filename.empty() || !(this->cur_file = FileHandle::Open(filename, "wb")).has_value()) { /* Unless that fails of course... */ CloseWindowById(WC_NETWORK_STATUS_WINDOW, WN_NETWORK_STATUS_WINDOW_CONTENT_DOWNLOAD); @@ -535,14 +528,14 @@ void ClientNetworkContentSocketHandler::AfterDownload() * Now gunzip the tar and make it known. */ this->cur_file.reset(); - if (GunzipFile(this->cur_info)) { - FioRemove(GetFullFilename(this->cur_info, true)); + if (GunzipFile(*this->cur_info)) { + FioRemove(GetFullFilename(*this->cur_info, true)); Subdirectory sd = GetContentInfoSubDir(this->cur_info->type); if (sd == NO_DIRECTORY) NOT_REACHED(); TarScanner ts; - std::string fname = GetFullFilename(this->cur_info, false); + std::string fname = GetFullFilename(*this->cur_info, false); ts.AddFile(sd, fname); if (this->cur_info->type == CONTENT_TYPE_BASE_MUSIC) { @@ -637,9 +630,8 @@ void ClientNetworkContentSocketHandler::OnReceiveData(std::unique_ptr da return; } - delete this->cur_info; /* When we haven't opened a file this must be our first packet with metadata. */ - this->cur_info = new ContentInfo; + this->cur_info = std::make_unique(); /** Check p for not being null and return calling OnFailure if that's not the case. */ #define check_not_null(p) { if ((p) == nullptr) { this->OnFailure(); return; } } @@ -732,9 +724,6 @@ ClientNetworkContentSocketHandler::ClientNetworkContentSocketHandler() : /** Clear up the mess ;) */ ClientNetworkContentSocketHandler::~ClientNetworkContentSocketHandler() { - delete this->cur_info; - - for (ContentInfo *ci : this->infos) delete ci; } /** Connect to the content server. */ @@ -844,8 +833,8 @@ void ClientNetworkContentSocketHandler::DownloadContentInfo(ContentID cid) */ ContentInfo *ClientNetworkContentSocketHandler::GetContent(ContentID cid) const { - for (ContentInfo *ci : this->infos) { - if (ci->id == cid) return ci; + for (const auto &ci : this->infos) { + if (ci->id == cid) return ci.get(); } return nullptr; } @@ -880,7 +869,7 @@ void ClientNetworkContentSocketHandler::Unselect(ContentID cid) /** Select everything we can select */ void ClientNetworkContentSocketHandler::SelectAll() { - for (ContentInfo *ci : this->infos) { + for (const auto &ci : this->infos) { if (ci->state == ContentInfo::UNSELECTED) { ci->state = ContentInfo::SELECTED; this->CheckDependencyState(*ci); @@ -891,7 +880,7 @@ void ClientNetworkContentSocketHandler::SelectAll() /** Select everything that's an update for something we've got */ void ClientNetworkContentSocketHandler::SelectUpgrade() { - for (ContentInfo *ci : this->infos) { + for (const auto &ci : this->infos) { if (ci->state == ContentInfo::UNSELECTED && ci->upgrade) { ci->state = ContentInfo::SELECTED; this->CheckDependencyState(*ci); @@ -902,7 +891,7 @@ void ClientNetworkContentSocketHandler::SelectUpgrade() /** Unselect everything that we've not downloaded so far. */ void ClientNetworkContentSocketHandler::UnselectAll() { - for (ContentInfo *ci : this->infos) { + for (const auto &ci : this->infos) { if (ci->IsSelected() && ci->state != ContentInfo::ALREADY_HERE) ci->state = ContentInfo::UNSELECTED; } } @@ -952,9 +941,9 @@ void ClientNetworkContentSocketHandler::ReverseLookupTreeDependency(ConstContent * we are including stuff into the vector and as such the vector's data * store can be reallocated (and thus move), which means out iterating * pointer gets invalid. So fall back to the indices. */ - for (uint i = 0; i < tree.size(); i++) { + for (const ContentInfo *ci : tree) { ConstContentVector parents; - this->ReverseLookupDependency(parents, *tree[i]); + this->ReverseLookupDependency(parents, *ci); for (const ContentInfo *ci : parents) { include(tree, ci); @@ -1056,8 +1045,6 @@ void ClientNetworkContentSocketHandler::CheckDependencyState(const ContentInfo & /** Clear all downloaded content information. */ void ClientNetworkContentSocketHandler::Clear() { - for (ContentInfo *c : this->infos) delete c; - this->infos.clear(); this->requested.clear(); this->reverse_dependency_map.clear(); diff --git a/src/network/network_content.h b/src/network/network_content.h index 0f09ee958c..e905b21b97 100644 --- a/src/network/network_content.h +++ b/src/network/network_content.h @@ -10,20 +10,16 @@ #ifndef NETWORK_CONTENT_H #define NETWORK_CONTENT_H +#include #include "core/tcp_content.h" #include "core/http.h" #include #include "../core/container_func.hpp" /** Vector with content info */ -typedef std::vector ContentVector; +using ContentVector = std::vector>; /** Vector with constant content info */ -typedef std::vector ConstContentVector; - -/** Iterator for the content vector */ -typedef ContentInfo **ContentIterator; -/** Iterator for the constant content vector */ -typedef const ContentInfo * const * ConstContentIterator; +using ConstContentVector = std::vector; /** Callbacks for notifying others about incoming data */ struct ContentCallback { @@ -66,7 +62,7 @@ struct ContentCallback { */ class ClientNetworkContentSocketHandler : public NetworkContentSocketHandler, ContentCallback, HTTPCallback { protected: - typedef std::vector ContentIDList; ///< List of content IDs to (possibly) select. + using ContentIDList = std::vector; ///< List of content IDs to (possibly) select. std::vector callbacks; ///< Callbacks to notify "the world" ContentIDList requested; ///< ContentIDs we already requested (so we don't do it again) ContentVector infos; ///< All content info we received @@ -75,7 +71,7 @@ protected: int http_response_index; ///< Where we are, in the response, with handling it std::optional cur_file; ///< Currently downloaded file - ContentInfo *cur_info; ///< Information about the currently downloaded file + std::unique_ptr cur_info; ///< Information about the currently downloaded file bool is_connecting; ///< Whether we're connecting bool is_cancelled; ///< Whether the download has been cancelled std::chrono::steady_clock::time_point last_activity; ///< The last time there was network activity @@ -132,14 +128,11 @@ public: void ReverseLookupTreeDependency(ConstContentVector &tree, const ContentInfo *child) const; void CheckDependencyState(const ContentInfo &ci); - /** Get the number of content items we know locally. */ - uint Length() const { return (uint)this->infos.size(); } - /** Get the begin of the content inf iterator. */ - ConstContentIterator Begin() const { return this->infos.data(); } - /** Get the nth position of the content inf iterator. */ - ConstContentIterator Get(uint32_t index) const { return this->infos.data() + index; } - /** Get the end of the content inf iterator. */ - ConstContentIterator End() const { return this->Begin() + this->Length(); } + /** + * Get a read-only view of content info for iterating externally. + * @return Read-only view of content info. + */ + auto Info() const { return this->infos | std::views::transform([](const auto &ci) -> const ContentInfo & { return *ci; }); } void Clear(); diff --git a/src/network/network_content_gui.cpp b/src/network/network_content_gui.cpp index c795ef8e0c..de77cc6dd7 100644 --- a/src/network/network_content_gui.cpp +++ b/src/network/network_content_gui.cpp @@ -410,9 +410,9 @@ class NetworkContentListWindow : public Window, ContentCallback { bool all_available = true; - for (ConstContentIterator iter = _network_content_client.Begin(); iter != _network_content_client.End(); iter++) { - if ((*iter)->state == ContentInfo::DOES_NOT_EXIST) all_available = false; - this->content.push_back(*iter); + for (const ContentInfo &ci : _network_content_client.Info()) { + if (ci.state == ContentInfo::DOES_NOT_EXIST) all_available = false; + this->content.push_back(&ci); } this->SetWidgetDisabledState(WID_NCL_SEARCH_EXTERNAL, this->auto_select && all_available); @@ -732,13 +732,11 @@ public: std::string buf; for (auto &cid : this->selected->dependencies) { /* Try to find the dependency */ - ConstContentIterator iter = _network_content_client.Begin(); - for (; iter != _network_content_client.End(); iter++) { - const ContentInfo *ci = *iter; - if (ci->id != cid) continue; + for (const ContentInfo &ci : _network_content_client.Info()) { + if (ci.id != cid) continue; if (!buf.empty()) buf += list_separator; - buf += (*iter)->name; + buf += ci.name; break; } } @@ -1138,9 +1136,5 @@ void ShowNetworkContentListWindow(ContentVector *cv, ContentType type1, ContentT GetEncodedString(STR_CONTENT_NO_ZLIB), GetEncodedString(STR_CONTENT_NO_ZLIB_SUB), WL_ERROR); - /* Connection failed... clean up the mess */ - if (cv != nullptr) { - for (ContentInfo *ci : *cv) delete ci; - } #endif /* WITH_ZLIB */ } diff --git a/src/newgrf_gui.cpp b/src/newgrf_gui.cpp index 9a36c269d2..2b145522d3 100644 --- a/src/newgrf_gui.cpp +++ b/src/newgrf_gui.cpp @@ -1517,13 +1517,13 @@ void ShowMissingContentWindow(const GRFConfigList &list) for (const auto &c : list) { if (c->status != GCS_NOT_FOUND && !c->flags.Test(GRFConfigFlag::Compatible)) continue; - ContentInfo *ci = new ContentInfo(); + auto ci = std::make_unique(); ci->type = CONTENT_TYPE_NEWGRF; ci->state = ContentInfo::DOES_NOT_EXIST; ci->name = c->GetName(); ci->unique_id = std::byteswap(c->ident.grfid); ci->md5sum = c->flags.Test(GRFConfigFlag::Compatible) ? c->original_md5sum : c->ident.md5sum; - cv.push_back(ci); + cv.push_back(std::move(ci)); } ShowNetworkContentListWindow(cv.empty() ? nullptr : &cv, CONTENT_TYPE_NEWGRF); }