From d3750f2b8ab851f1d2f21e56e7c529fb678b3433 Mon Sep 17 00:00:00 2001 From: ohyzha Date: Sat, 21 Dec 2024 17:11:29 +0200 Subject: [PATCH] code review refactoring --- .../Host/ExeAppendedZipResourceLoader.cpp | 41 ++-------- .../Host/ExeAppendedZipResourceLoader.hpp | 6 +- .../ExeAppendedZipResourceLoaderLinux.cpp | 18 ++--- .../ExeAppendedZipResourceLoaderLinux.hpp | 4 +- .../ExeAppendedZipResourceLoaderWindows.cpp | 7 +- openVulkanoCpp/IO/Archive/ArchiveReader.cpp | 75 +++++++++++-------- openVulkanoCpp/IO/Archive/ArchiveReader.hpp | 14 ++-- openVulkanoCpp/IO/Archive/ArchiveType.hpp | 2 +- tests/CMakeLists.txt | 4 +- tests/Host/ExeAppendedZipLoaderTests.cpp | 34 +++------ 10 files changed, 88 insertions(+), 117 deletions(-) diff --git a/openVulkanoCpp/Host/ExeAppendedZipResourceLoader.cpp b/openVulkanoCpp/Host/ExeAppendedZipResourceLoader.cpp index 1fa0bac..a9c311d 100644 --- a/openVulkanoCpp/Host/ExeAppendedZipResourceLoader.cpp +++ b/openVulkanoCpp/Host/ExeAppendedZipResourceLoader.cpp @@ -7,50 +7,21 @@ #include "ExeAppendedZipResourceLoader.hpp" #include "Base/Logger.hpp" #include "IO/Archive/ArchiveReader.hpp" +#include "Base/Utils.hpp" #include #include namespace OpenVulkano { - std::string ExeAppendedZipResourceLoader::GetResourcePath(const std::string& resourceName) + Array ExeAppendedZipResourceLoader::GetResource(const std::string& resourceName) { - return GetCurrentExecutablePath(); - } - - Array ExeAppendedZipResourceLoader::GetResource(const std::string& exeName) - { - std::string ext = std::filesystem::path(exeName).extension().string(); - if (ext != ".exe" && ext != "") - { - Logger::DATA->debug("Given file {} is not a valid executable", exeName); - return {}; - } - - std::ifstream ifs(exeName, std::ios::in | std::ios::binary); - if (!ifs.is_open()) - { - Logger::DATA->debug("Could not open file {}.", exeName); - return {}; - } - ifs.seekg(0, std::ios_base::end); - size_t zipSize = ifs.tellg(); - ifs.seekg(0, std::ios_base::beg); - Array zipData(zipSize); - ifs.read(zipData.Data(), zipSize); - ifs.close(); - return zipData; - } - - std::vector>> ExeAppendedZipResourceLoader::GetZipArchiveFiles(const std::string& exePath) - { - Array zipData = GetResource(exePath); + Array zipData = Utils::ReadFile(GetCurrentExecutablePath()); ArchiveReader reader(zipData.Data(), zipData.Size(), nullptr, ArchiveType::ZIP); - std::vector>> filesFromZip; - while (reader.HasNext()) + if (auto data = reader.GetFile(resourceName)) { - filesFromZip.push_back(*reader.GetNextFile()); + return data->second; } - return filesFromZip; + return {}; } } \ No newline at end of file diff --git a/openVulkanoCpp/Host/ExeAppendedZipResourceLoader.hpp b/openVulkanoCpp/Host/ExeAppendedZipResourceLoader.hpp index c9021d7..a5a1863 100644 --- a/openVulkanoCpp/Host/ExeAppendedZipResourceLoader.hpp +++ b/openVulkanoCpp/Host/ExeAppendedZipResourceLoader.hpp @@ -16,9 +16,9 @@ namespace OpenVulkano class ExeAppendedZipResourceLoader : public ResourceLoader { public: - std::string GetResourcePath(const std::string& resourceName) override; - Array GetResource(const std::string& exePath) override; - std::vector>> GetZipArchiveFiles(const std::string& exePath); + std::string GetResourcePath(const std::string& resourceName) final { return ""; } + Array GetResource(const std::string& resourceName) override; + protected: virtual std::string GetCurrentExecutablePath() const = 0; }; } \ No newline at end of file diff --git a/openVulkanoCpp/Host/Linux/ExeAppendedZipResourceLoaderLinux.cpp b/openVulkanoCpp/Host/Linux/ExeAppendedZipResourceLoaderLinux.cpp index 90332b9..9f9f52b 100644 --- a/openVulkanoCpp/Host/Linux/ExeAppendedZipResourceLoaderLinux.cpp +++ b/openVulkanoCpp/Host/Linux/ExeAppendedZipResourceLoaderLinux.cpp @@ -4,7 +4,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -#include "ExeAppendedZipLoaderLinux.hpp" +#include "ExeAppendedZipResourceLoaderLinux.hpp" #include #include @@ -12,19 +12,15 @@ namespace OpenVulkano { namespace { - void* HANDLE = ResourceLoader::RegisterResourceLoader(std::make_unique()); + void* HANDLE = ResourceLoader::RegisterResourceLoader(std::make_unique()); } - std::string OpenVulkano::ExeAppendedZipLoaderLinux::GetCurrentExecutablePath() const + std::string OpenVulkano::ExeAppendedZipResourceLoaderLinux::GetCurrentExecutablePath() const { - char dest[PATH_MAX]; - memset(dest, 0, sizeof(dest)); // readlink does not null terminate! - size_t sz = 0; - if ((sz = readlink("/proc/self/exe", dest, PATH_MAX)) != -1) - { - return std::string(dest, sz); - } - return ""; + std::string path(PATH_MAX, '\0'); + ssize_t sz = readlink("/proc/self/exe", path.data(), path.capacity())); + path.resize(std::max(0, sz)); + return path; } } \ No newline at end of file diff --git a/openVulkanoCpp/Host/Linux/ExeAppendedZipResourceLoaderLinux.hpp b/openVulkanoCpp/Host/Linux/ExeAppendedZipResourceLoaderLinux.hpp index c1ce15e..73cfe31 100644 --- a/openVulkanoCpp/Host/Linux/ExeAppendedZipResourceLoaderLinux.hpp +++ b/openVulkanoCpp/Host/Linux/ExeAppendedZipResourceLoaderLinux.hpp @@ -6,11 +6,11 @@ #pragma once -#include "Host/ExeAppendedZipLoader.hpp" +#include "Host/ExeAppendedZipResourceLoader.hpp" namespace OpenVulkano { - class ExeAppendedZipLoaderLinux final : public ExeAppendedZipLoader + class ExeAppendedZipResourceLoaderLinux final : public ExeAppendedZipResourceLoader { public: std::string GetCurrentExecutablePath() const override; diff --git a/openVulkanoCpp/Host/Windows/ExeAppendedZipResourceLoaderWindows.cpp b/openVulkanoCpp/Host/Windows/ExeAppendedZipResourceLoaderWindows.cpp index 92dadc6..8e50bda 100644 --- a/openVulkanoCpp/Host/Windows/ExeAppendedZipResourceLoaderWindows.cpp +++ b/openVulkanoCpp/Host/Windows/ExeAppendedZipResourceLoaderWindows.cpp @@ -16,9 +16,10 @@ namespace OpenVulkano std::string OpenVulkano::ExeAppendedZipResourceLoaderWindows::GetCurrentExecutablePath() const { - CHAR nameBuf[MAX_PATH] = {}; - DWORD len = GetModuleFileNameA(NULL, nameBuf, MAX_PATH); - return std::string(nameBuf, len); + std::string exe(MAX_PATH, '\0'); + DWORD len = GetModuleFileNameA(NULL, exe.data(), MAX_PATH); + exe.resize(std::max(len, exe.size())); + return exe; } } \ No newline at end of file diff --git a/openVulkanoCpp/IO/Archive/ArchiveReader.cpp b/openVulkanoCpp/IO/Archive/ArchiveReader.cpp index 47f9e82..0ce6a92 100644 --- a/openVulkanoCpp/IO/Archive/ArchiveReader.cpp +++ b/openVulkanoCpp/IO/Archive/ArchiveReader.cpp @@ -20,21 +20,21 @@ namespace OpenVulkano constexpr int BUFFER_SIZE = 16384; } - ArchiveReader::ArchiveReader(const std::shared_ptr& logger, ArchiveType::Type archiveType) + ArchiveReader::ArchiveReader(const std::shared_ptr& logger, std::optional archiveType) : ArchiveBase(archive_read_new(), nullptr, logger), m_archiveType(archiveType) {} - ArchiveReader::ArchiveReader(const std::string& archiveFile, const std::shared_ptr& logger, ArchiveType::Type archiveType) + ArchiveReader::ArchiveReader(const std::string& archiveFile, const std::shared_ptr& logger, std::optional archiveType) : ArchiveReader(archiveFile.c_str(), logger, archiveType) {} - ArchiveReader::ArchiveReader(const char* archiveFile, const std::shared_ptr& logger, ArchiveType::Type archiveType) + ArchiveReader::ArchiveReader(const char* archiveFile, const std::shared_ptr& logger, std::optional archiveType) : ArchiveReader(logger, archiveType) { Open(archiveFile); } - ArchiveReader::ArchiveReader(const void* archiveBuffer, const size_t size, const std::shared_ptr& logger, ArchiveType::Type archiveType) + ArchiveReader::ArchiveReader(const void* archiveBuffer, const size_t size, const std::shared_ptr& logger, std::optional archiveType) : ArchiveReader(logger, archiveType) { OpenMemory(archiveBuffer, size); @@ -51,32 +51,7 @@ namespace OpenVulkano m_archive = archive_read_new(); } ChkErr(archive_read_support_filter_all(m_archive)); - std::function pFunc; - switch (m_archiveType) - { - case ArchiveType::ZIP: - pFunc = archive_read_support_format_zip; - break; - case ArchiveType::SEVEN_ZIP: - pFunc = archive_read_support_format_7zip; - break; - case ArchiveType::CPIO: - pFunc = archive_read_support_format_cpio; - break; - case ArchiveType::ISO: - pFunc = archive_read_support_format_iso9660; - case ArchiveType::TAR: - pFunc = archive_read_support_format_tar; - break; - case ArchiveType::WARC: - pFunc = archive_read_support_format_warc; - break; - case ArchiveType::XAR: - pFunc = archive_read_support_format_xar; - break; - default: - pFunc = archive_read_support_format_all; - } + std::function pFunc = GetCurrentFormatReadFunc(); ChkErr(pFunc(m_archive)); m_open = true; } @@ -182,6 +157,33 @@ namespace OpenVulkano } } + std::function ArchiveReader::GetCurrentFormatReadFunc() const + { + if (!m_archiveType) + { + return archive_read_support_format_all; + } + ArchiveType::Type type = *m_archiveType; + switch (type) + { + case ArchiveType::ZIP: + return archive_read_support_format_zip; + case ArchiveType::SEVEN_ZIP: + return archive_read_support_format_7zip; + case ArchiveType::CPIO: + return archive_read_support_format_cpio; + case ArchiveType::ISO: + return archive_read_support_format_iso9660; + case ArchiveType::TAR: + return archive_read_support_format_tar; + case ArchiveType::WARC: + return archive_read_support_format_warc; + case ArchiveType::XAR: + return archive_read_support_format_xar; + } + return archive_read_support_format_all; + } + size_t ArchiveReader::ExtractRemaining(std::string_view targetDir) { size_t count = 0; @@ -289,6 +291,19 @@ namespace OpenVulkano return std::nullopt; } + std::optional>> ArchiveReader::GetFile(const std::string& path) + { + while (HasNext()) + { + const std::pair> info = *GetNextFile(); + if (info.first.path == path) + { + return info; + } + } + return {}; + } + bool ArchiveReader::GetNextFileAsStream(const std::function& streamReader) { if (SkipTill(std::filesystem::file_type::regular)) diff --git a/openVulkanoCpp/IO/Archive/ArchiveReader.hpp b/openVulkanoCpp/IO/Archive/ArchiveReader.hpp index 7b78048..b2f0642 100644 --- a/openVulkanoCpp/IO/Archive/ArchiveReader.hpp +++ b/openVulkanoCpp/IO/Archive/ArchiveReader.hpp @@ -21,17 +21,17 @@ namespace OpenVulkano { bool m_open = false; bool m_eof = false; - ArchiveType::Type m_archiveType; + std::optional m_archiveType; std::queue m_archivesToRead; public: - explicit ArchiveReader(const std::shared_ptr& logger = nullptr, ArchiveType::Type archiveType = ArchiveType::ANY); + explicit ArchiveReader(const std::shared_ptr& logger = nullptr, std::optional archiveType = std::nullopt); - explicit ArchiveReader(const char* archiveFile, const std::shared_ptr& logger = nullptr, ArchiveType::Type archiveType = ArchiveType::ANY); + explicit ArchiveReader(const char* archiveFile, const std::shared_ptr& logger = nullptr, std::optional archiveType = std::nullopt); - explicit ArchiveReader(const std::string& archiveFile, const std::shared_ptr& logger = nullptr, ArchiveType::Type archiveType = ArchiveType::ANY); + explicit ArchiveReader(const std::string& archiveFile, const std::shared_ptr& logger = nullptr, std::optional archiveType = std::nullopt); - ArchiveReader(const void* archiveBuffer, size_t size, const std::shared_ptr& logger = nullptr, ArchiveType::Type archiveType = ArchiveType::ANY); + ArchiveReader(const void* archiveBuffer, size_t size, const std::shared_ptr& logger = nullptr, std::optional archiveType = std::nullopt); ~ArchiveReader() override; @@ -66,13 +66,15 @@ namespace OpenVulkano std::optional>> GetNextFileAsVector(); + std::optional>> GetFile(const std::string& path); + std::optional StreamNextFile(std::ostream& stream); bool GetNextFileAsStream(const std::function& streamReader); private: void ReadNextHeader(); - + std::function GetCurrentFormatReadFunc() const; void PrepOpen(); }; } diff --git a/openVulkanoCpp/IO/Archive/ArchiveType.hpp b/openVulkanoCpp/IO/Archive/ArchiveType.hpp index 91bdc03..739ba1f 100644 --- a/openVulkanoCpp/IO/Archive/ArchiveType.hpp +++ b/openVulkanoCpp/IO/Archive/ArchiveType.hpp @@ -17,7 +17,7 @@ namespace OpenVulkano static inline constexpr std::string_view TYPE_NAMES[] = { ".tar", ".cpio", ".iso", ".zip", ".xar", ".7z", ".warc", ".shar" }; public: - enum Type { TAR = 0, CPIO, ISO, ZIP, XAR, SEVEN_ZIP, WARC, SHAR, ANY }; + enum Type { TAR = 0, CPIO, ISO, ZIP, XAR, SEVEN_ZIP, WARC, SHAR }; constexpr ArchiveType(Type type) : m_type(type) {} diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 19d78dc..85c0c4f 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -12,7 +12,7 @@ if (NOT ENABLE_CURL) endif() if (APPLE) - list(FILTER SOURCES EXCLUDE REGEX "ExeAppendedZipLoader") + list(FILTER SOURCES EXCLUDE REGEX "ExeAppendedZipResourceLoader") endif() source_group(TREE "${CMAKE_CURRENT_SOURCE_DIR}" FILES ${SOURCES}) @@ -21,7 +21,7 @@ list(APPEND SOURCES ${RESOURCES}) add_executable(OpenVulkano_Tests ${SOURCES}) # append zip file at the end of executable file -if (WIN32 OR (LINUX AND NOT APPLE)) +if (WIN32 OR LINUX) set(ZIP_FILE ${ROOT_FOLDER}/resources/arch.zip) add_custom_command(TARGET OpenVulkano_Tests POST_BUILD COMMAND ${CMAKE_COMMAND} -E cat ${ZIP_FILE} >> $) endif() diff --git a/tests/Host/ExeAppendedZipLoaderTests.cpp b/tests/Host/ExeAppendedZipLoaderTests.cpp index 5923cef..16ff92f 100644 --- a/tests/Host/ExeAppendedZipLoaderTests.cpp +++ b/tests/Host/ExeAppendedZipLoaderTests.cpp @@ -7,9 +7,9 @@ #include #ifdef _WIN32 - #include "Host/Windows/ExeAppendedZipLoaderWindows.hpp" + #include "Host/Windows/ExeAppendedZipResourceLoaderWindows.hpp" #else - #include "Host/Linux/ExeAppendedZipLoaderLinux.hpp" + #include "Host/Linux/ExeAppendedZipResourceLoaderLinux.hpp" #endif #include "Base/Logger.hpp" @@ -23,30 +23,16 @@ TEST_CASE("Load zip from exe") { Logger::SetupLogger("", "tests.log"); #ifdef _WIN32 - ExeAppendedZipLoaderWindows loader; + ExeAppendedZipResourceLoaderWindows loader; #else - ExeAppendedZipLoaderLinux loader; + ExeAppendedZipResourceLoaderLinux loader; #endif - Array zipData = loader.GetResource(loader.GetCurrentExecutablePath()); - REQUIRE(!zipData.Empty()); + auto iconData = loader.GetResource("madvoxel_icon.ico"); + REQUIRE(!iconData.Empty()); - auto files = loader.GetZipArchiveFiles(loader.GetCurrentExecutablePath()); - REQUIRE(files.size() == 2); - - int i = 0; - for (const auto& [fileDesc, fileData] : files) - { - if (i == 0) - { - REQUIRE(fileDesc.path == "madvoxel_icon.ico"); - } - else if (i == 1) - { - REQUIRE(fileDesc.path == "text.txt"); - std::string s(fileData.Data(), fileData.Size()); - REQUIRE(s == "Hello world!"); - } - i++; - } + auto txtFile = loader.GetResource("text.txt"); + REQUIRE(!txtFile.Empty()); + std::string s(txtFile.Data(), txtFile.Size()); + REQUIRE(s == "Hello world!"); }