From 57701cd988c3c88cc0a9295f2ad2c55c5b29dc82 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 27 May 2019 12:33:46 -0400 Subject: [PATCH 1/6] UICommon/ResourcePack: Make TEXTURE_PATH a regular array Same behavior, only it doesn't unnecessarily store a pointer in the executable. While we're at it, make it constexpr and move it into the namespace. --- Source/Core/UICommon/ResourcePack/ResourcePack.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Source/Core/UICommon/ResourcePack/ResourcePack.cpp b/Source/Core/UICommon/ResourcePack/ResourcePack.cpp index e52034f508..4f4169d053 100644 --- a/Source/Core/UICommon/ResourcePack/ResourcePack.cpp +++ b/Source/Core/UICommon/ResourcePack/ResourcePack.cpp @@ -15,10 +15,10 @@ #include "UICommon/ResourcePack/Manager.h" #include "UICommon/ResourcePack/Manifest.h" -static const char* TEXTURE_PATH = "Load/Textures/"; - namespace ResourcePack { +constexpr char TEXTURE_PATH[] = "Load/Textures/"; + // Since minzip doesn't provide a way to unzip a file of a length > 65535, we have to implement // this ourselves static bool ReadCurrentFileUnlimited(unzFile file, std::vector& destination) From a22cc615a97a0e7d55f44e8d14428418def58e2d Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 27 May 2019 12:37:43 -0400 Subject: [PATCH 2/6] UICommon/ResourcePack: Remove unnecessary resizes We can simply construct the containers with the desired size in these cases. --- .../UICommon/ResourcePack/ResourcePack.cpp | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/Source/Core/UICommon/ResourcePack/ResourcePack.cpp b/Source/Core/UICommon/ResourcePack/ResourcePack.cpp index 4f4169d053..29bf41c9e0 100644 --- a/Source/Core/UICommon/ResourcePack/ResourcePack.cpp +++ b/Source/Core/UICommon/ResourcePack/ResourcePack.cpp @@ -68,13 +68,9 @@ ResourcePack::ResourcePack(const std::string& path) : m_path(path) } unz_file_info manifest_info; - unzGetCurrentFileInfo(file, &manifest_info, nullptr, 0, nullptr, 0, nullptr, 0); - std::vector manifest_contents; - - manifest_contents.resize(manifest_info.uncompressed_size); - + std::vector manifest_contents(manifest_info.uncompressed_size); if (!ReadCurrentFileUnlimited(file, manifest_contents)) { m_valid = false; @@ -114,13 +110,10 @@ ResourcePack::ResourcePack(const std::string& path) : m_path(path) do { - std::string filename; - - filename.resize(256); + std::string filename(256, '\0'); unz_file_info texture_info; - - unzGetCurrentFileInfo(file, &texture_info, &filename[0], static_cast(filename.size()), + unzGetCurrentFileInfo(file, &texture_info, filename.data(), static_cast(filename.size()), nullptr, 0, nullptr, 0); if (filename.compare(0, 9, "textures/") != 0 || texture_info.uncompressed_size == 0) @@ -215,12 +208,9 @@ bool ResourcePack::Install(const std::string& path) } unz_file_info texture_info; - unzGetCurrentFileInfo(file, &texture_info, nullptr, 0, nullptr, 0, nullptr, 0); - std::vector data; - data.resize(texture_info.uncompressed_size); - + std::vector data(texture_info.uncompressed_size); if (!ReadCurrentFileUnlimited(file, data)) { m_error = "Failed to read texture " + texture; From 157a305507cbeac1d1557f0d7767dc57807c1945 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 27 May 2019 12:45:21 -0400 Subject: [PATCH 3/6] UICommon/ResourcePack: Deduplicate string construction A few cases duplicate the string patch creation, which is kind of wasteful. We can just construct the string once. --- Source/Core/UICommon/ResourcePack/ResourcePack.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Source/Core/UICommon/ResourcePack/ResourcePack.cpp b/Source/Core/UICommon/ResourcePack/ResourcePack.cpp index 29bf41c9e0..5a5e443912 100644 --- a/Source/Core/UICommon/ResourcePack/ResourcePack.cpp +++ b/Source/Core/UICommon/ResourcePack/ResourcePack.cpp @@ -197,9 +197,9 @@ bool ResourcePack::Install(const std::string& path) return false; } + const std::string texture_path = path + TEXTURE_PATH + texture; std::string m_full_dir; - - SplitPath(path + TEXTURE_PATH + texture, &m_full_dir, nullptr, nullptr); + SplitPath(texture_path, &m_full_dir, nullptr, nullptr); if (!File::CreateFullPath(m_full_dir)) { @@ -217,7 +217,7 @@ bool ResourcePack::Install(const std::string& path) return false; } - std::ofstream out(path + TEXTURE_PATH + texture, std::ios::trunc | std::ios::binary); + std::ofstream out(texture_path, std::ios::trunc | std::ios::binary); if (!out.good()) { @@ -284,7 +284,8 @@ bool ResourcePack::Uninstall(const std::string& path) if (provided_by_other_pack) continue; - if (File::Exists(path + TEXTURE_PATH + texture) && !File::Delete(path + TEXTURE_PATH + texture)) + const std::string texture_path = path + TEXTURE_PATH + texture; + if (File::Exists(texture_path) && !File::Delete(texture_path)) { m_error = "Failed to delete texture " + texture; return false; @@ -293,8 +294,7 @@ bool ResourcePack::Uninstall(const std::string& path) // Recursively delete empty directories std::string dir; - - SplitPath(path + TEXTURE_PATH + texture, &dir, nullptr, nullptr); + SplitPath(texture_path, &dir, nullptr, nullptr); while (dir.length() > (path + TEXTURE_PATH).length()) { From 41cda6fe6dcf85abf4c2b1e3430e3e179477fa9d Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 27 May 2019 13:02:01 -0400 Subject: [PATCH 4/6] UICommon/ResourcePack: Use ScopeGuards to manage closing files Makes it way harder to introduce resource leaks, and plugs the existing resource leaks in the constructor and Install() where the file wouldn't be closed in some error cases. --- .../UICommon/ResourcePack/ResourcePack.cpp | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/Source/Core/UICommon/ResourcePack/ResourcePack.cpp b/Source/Core/UICommon/ResourcePack/ResourcePack.cpp index 5a5e443912..72cf68651d 100644 --- a/Source/Core/UICommon/ResourcePack/ResourcePack.cpp +++ b/Source/Core/UICommon/ResourcePack/ResourcePack.cpp @@ -10,6 +10,7 @@ #include "Common/FileSearch.h" #include "Common/FileUtil.h" +#include "Common/ScopeGuard.h" #include "Common/StringUtil.h" #include "UICommon/ResourcePack/Manager.h" @@ -23,35 +24,34 @@ constexpr char TEXTURE_PATH[] = "Load/Textures/"; // this ourselves static bool ReadCurrentFileUnlimited(unzFile file, std::vector& destination) { - const uint32_t MAX_BUFFER_SIZE = 65535; + const u32 MAX_BUFFER_SIZE = 65535; if (unzOpenCurrentFile(file) != UNZ_OK) return false; - uint32_t bytes_to_go = static_cast(destination.size()); + Common::ScopeGuard guard{[&] { unzCloseCurrentFile(file); }}; + auto bytes_to_go = static_cast(destination.size()); while (bytes_to_go > 0) { - int bytes_read = unzReadCurrentFile(file, &destination[destination.size() - bytes_to_go], - std::min(bytes_to_go, MAX_BUFFER_SIZE)); + const int bytes_read = unzReadCurrentFile(file, &destination[destination.size() - bytes_to_go], + std::min(bytes_to_go, MAX_BUFFER_SIZE)); if (bytes_read < 0) { - unzCloseCurrentFile(file); return false; } - bytes_to_go -= bytes_read; + bytes_to_go -= static_cast(bytes_read); } - unzCloseCurrentFile(file); - return true; } ResourcePack::ResourcePack(const std::string& path) : m_path(path) { auto file = unzOpen(path.c_str()); + Common::ScopeGuard file_guard{[&] { unzClose(file); }}; if (file == nullptr) { @@ -129,8 +129,6 @@ ResourcePack::ResourcePack(const std::string& path) : m_path(path) m_textures.push_back(filename.substr(9)); } while (unzGoToNextFile(file) != UNZ_END_OF_LIST_OF_FILE); - - unzClose(file); } bool ResourcePack::IsValid() const @@ -172,6 +170,7 @@ bool ResourcePack::Install(const std::string& path) } auto file = unzOpen(m_path.c_str()); + Common::ScopeGuard file_guard{[&] { unzClose(file); }}; for (const auto& texture : m_textures) { @@ -229,10 +228,7 @@ bool ResourcePack::Install(const std::string& path) out.flush(); } - unzClose(file); - SetInstalled(*this, true); - return true; } From 5c4d3f55da0327721d1f2494752f4dc47924682d Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 27 May 2019 13:09:21 -0400 Subject: [PATCH 5/6] UICommon/Manager: Remove unused std::string variable in Remove() --- Source/Core/UICommon/ResourcePack/Manager.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/Source/Core/UICommon/ResourcePack/Manager.cpp b/Source/Core/UICommon/ResourcePack/Manager.cpp index a95d60a280..4977f09523 100644 --- a/Source/Core/UICommon/ResourcePack/Manager.cpp +++ b/Source/Core/UICommon/ResourcePack/Manager.cpp @@ -143,8 +143,6 @@ bool Remove(ResourcePack& pack) if (pack_iterator == packs.end()) return false; - std::string filename; - IniFile file = GetPackConfig(); auto* order = file.GetOrCreateSection("Order"); From f07cf9ebab36548f4319ba8e3ffc0fa77948d1f6 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 27 May 2019 13:27:03 -0400 Subject: [PATCH 6/6] UICommon/ResourcePack: Allow ReadCurrentFileUnlimited() to read into any contiguous container This allows the same code to be used to read into a std::string, which allows for eliminating the vector->string transfer when reading the manifest file. A ContiguousContainer is a concept that includes std::array, std::string, and std::vector. --- Source/Core/UICommon/ResourcePack/ResourcePack.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/Source/Core/UICommon/ResourcePack/ResourcePack.cpp b/Source/Core/UICommon/ResourcePack/ResourcePack.cpp index 72cf68651d..24c6f7c76e 100644 --- a/Source/Core/UICommon/ResourcePack/ResourcePack.cpp +++ b/Source/Core/UICommon/ResourcePack/ResourcePack.cpp @@ -22,7 +22,8 @@ constexpr char TEXTURE_PATH[] = "Load/Textures/"; // Since minzip doesn't provide a way to unzip a file of a length > 65535, we have to implement // this ourselves -static bool ReadCurrentFileUnlimited(unzFile file, std::vector& destination) +template +static bool ReadCurrentFileUnlimited(unzFile file, ContiguousContainer& destination) { const u32 MAX_BUFFER_SIZE = 65535; @@ -70,19 +71,16 @@ ResourcePack::ResourcePack(const std::string& path) : m_path(path) unz_file_info manifest_info; unzGetCurrentFileInfo(file, &manifest_info, nullptr, 0, nullptr, 0, nullptr, 0); - std::vector manifest_contents(manifest_info.uncompressed_size); + std::string manifest_contents(manifest_info.uncompressed_size, '\0'); if (!ReadCurrentFileUnlimited(file, manifest_contents)) { m_valid = false; m_error = "Failed to read manifest.json"; return; } - unzCloseCurrentFile(file); - m_manifest = - std::make_shared(std::string(manifest_contents.begin(), manifest_contents.end())); - + m_manifest = std::make_shared(manifest_contents); if (!m_manifest->IsValid()) { m_valid = false;