From 04aa351c40a19c67382da5470a37b79df477367a Mon Sep 17 00:00:00 2001 From: Hamish Milne <hamishmilne83@gmail.com> Date: Sun, 29 Mar 2020 16:14:36 +0100 Subject: [PATCH] Code review - general gardening --- src/common/construct.h | 15 ++++----- src/common/memory_ref.h | 49 ++++++++++++++++-------------- src/common/thread_queue_list.h | 10 +++--- src/core/arm/arm_interface.h | 38 +++++++++++------------ src/core/hle/kernel/config_mem.h | 8 +++-- src/core/hle/kernel/kernel.cpp | 6 ++-- src/core/hle/kernel/shared_page.h | 10 ++++-- src/core/hle/service/cecd/cecd.cpp | 43 +++++++++++++++----------- src/core/hle/service/http_c.cpp | 4 +-- src/core/memory.cpp | 30 ++++++++++++++---- 10 files changed, 125 insertions(+), 88 deletions(-) diff --git a/src/common/construct.h b/src/common/construct.h index 4e48ee60d..cb47bb46e 100644 --- a/src/common/construct.h +++ b/src/common/construct.h @@ -6,28 +6,29 @@ #include <boost/serialization/serialization.hpp> +/// Allows classes to define `save_construct` and `load_construct` methods for serialization +/// This is used where we don't call the default constructor during deserialization, as a shortcut +/// instead of using load_construct_data directly class construct_access { public: template <class Archive, class T> - static inline void save_construct(Archive& ar, const T* t, const unsigned int file_version) { + static void save_construct(Archive& ar, const T* t, const unsigned int file_version) { t->save_construct(ar, file_version); } template <class Archive, class T> - static inline void load_construct(Archive& ar, T* t, const unsigned int file_version) { + static void load_construct(Archive& ar, T* t, const unsigned int file_version) { T::load_construct(ar, t, file_version); } }; #define BOOST_SERIALIZATION_CONSTRUCT(T) \ - namespace boost { \ - namespace serialization { \ + namespace boost::serialization { \ template <class Archive> \ - inline void save_construct_data(Archive& ar, const T* t, const unsigned int file_version) { \ + void save_construct_data(Archive& ar, const T* t, const unsigned int file_version) { \ construct_access::save_construct(ar, t, file_version); \ } \ template <class Archive> \ - inline void load_construct_data(Archive& ar, T* t, const unsigned int file_version) { \ + void load_construct_data(Archive& ar, T* t, const unsigned int file_version) { \ construct_access::load_construct(ar, t, file_version); \ } \ - } \ } diff --git a/src/common/memory_ref.h b/src/common/memory_ref.h index 65946fc58..30eabbaee 100644 --- a/src/common/memory_ref.h +++ b/src/common/memory_ref.h @@ -17,7 +17,8 @@ class BackingMem { public: virtual ~BackingMem() = default; virtual u8* GetPtr() = 0; - virtual u32 GetSize() const = 0; + virtual const u8* GetPtr() const = 0; + virtual std::size_t GetSize() const = 0; private: template <class Archive> @@ -35,7 +36,11 @@ public: return data.data(); } - u32 GetSize() const override { + const u8* GetPtr() const override { + return data.data(); + } + + std::size_t GetSize() const override { return static_cast<u32>(data.size()); } @@ -66,51 +71,51 @@ public: : backing_mem(std::move(backing_mem_)), offset(0) { Init(); } - MemoryRef(std::shared_ptr<BackingMem> backing_mem_, u32 offset_) + MemoryRef(std::shared_ptr<BackingMem> backing_mem_, u64 offset_) : backing_mem(std::move(backing_mem_)), offset(offset_) { ASSERT(offset < backing_mem->GetSize()); Init(); } - inline operator u8*() { - return cptr; - } - inline u8* GetPtr() { - return cptr; - } explicit operator bool() const { return cptr != nullptr; } - inline const u8* GetPtr() const { + operator u8*() { return cptr; } - inline u32 GetSize() const { + u8* GetPtr() { + return cptr; + } + operator const u8*() const { + return cptr; + } + const u8* GetPtr() const { + return cptr; + } + std::size_t GetSize() const { return csize; } - inline void operator+=(u32 offset_by) { + MemoryRef& operator+=(u32 offset_by) { ASSERT(offset_by < csize); offset += offset_by; Init(); + return *this; } - inline MemoryRef operator+(u32 offset_by) const { + MemoryRef operator+(u32 offset_by) const { ASSERT(offset_by < csize); return MemoryRef(backing_mem, offset + offset_by); } - inline u8* operator+(std::size_t offset_by) const { - ASSERT(offset_by < csize); - return cptr + offset_by; - } private: - std::shared_ptr<BackingMem> backing_mem = nullptr; - u32 offset = 0; + std::shared_ptr<BackingMem> backing_mem{}; + u64 offset{}; // Cached values for speed - u8* cptr = nullptr; - u32 csize = 0; + u8* cptr{}; + std::size_t csize{}; void Init() { if (backing_mem) { cptr = backing_mem->GetPtr() + offset; - csize = static_cast<u32>(backing_mem->GetSize() - offset); + csize = static_cast<std::size_t>(backing_mem->GetSize() - offset); } else { cptr = nullptr; csize = 0; diff --git a/src/common/thread_queue_list.h b/src/common/thread_queue_list.h index abeb465ad..1ba68e1e4 100644 --- a/src/common/thread_queue_list.h +++ b/src/common/thread_queue_list.h @@ -161,7 +161,7 @@ private: // The priority level queues of thread ids. std::array<Queue, NUM_QUEUES> queues; - s32 ToIndex(Queue* q) const { + s64 ToIndex(const Queue* q) const { if (q == nullptr) { return -2; } else if (q == UnlinkedTag()) { @@ -171,7 +171,7 @@ private: } } - Queue* ToPointer(s32 idx) { + Queue* ToPointer(s64 idx) { if (idx == -1) { return UnlinkedTag(); } else if (idx < 0) { @@ -184,10 +184,10 @@ private: friend class boost::serialization::access; template <class Archive> void save(Archive& ar, const unsigned int file_version) const { - s32 idx = ToIndex(first); + const s64 idx = ToIndex(first); ar << idx; for (size_t i = 0; i < NUM_QUEUES; i++) { - const s32 idx1 = ToIndex(queues[i].next_nonempty); + const s64 idx1 = ToIndex(queues[i].next_nonempty); ar << idx1; ar << queues[i].data; } @@ -195,7 +195,7 @@ private: template <class Archive> void load(Archive& ar, const unsigned int file_version) { - s32 idx; + s64 idx; ar >> idx; first = ToPointer(idx); for (auto i = 0; i < NUM_QUEUES; i++) { diff --git a/src/core/arm/arm_interface.h b/src/core/arm/arm_interface.h index e39e1b9fa..fca488586 100644 --- a/src/core/arm/arm_interface.h +++ b/src/core/arm/arm_interface.h @@ -36,11 +36,11 @@ public: const auto r = GetFpuRegister(i); ar << r; } - auto r1 = GetCpsr(); + const auto r1 = GetCpsr(); ar << r1; - auto r2 = GetFpscr(); + const auto r2 = GetFpscr(); ar << r2; - auto r3 = GetFpexc(); + const auto r3 = GetFpexc(); ar << r3; } @@ -245,26 +245,26 @@ private: void save(Archive& ar, const unsigned int file_version) const { ar << timer; ar << id; - auto page_table = GetPageTable(); + const auto page_table = GetPageTable(); ar << page_table; - for (size_t i = 0; i < 15; i++) { - auto r = GetReg(i); + for (int i = 0; i < 15; i++) { + const auto r = GetReg(i); ar << r; } - auto pc = GetPC(); + const auto pc = GetPC(); ar << pc; - auto cpsr = GetCPSR(); + const auto cpsr = GetCPSR(); ar << cpsr; - for (size_t i = 0; i < 32; i++) { - auto r = GetVFPReg(i); + for (int i = 0; i < 32; i++) { + const auto r = GetVFPReg(i); ar << r; } - for (auto i = 0; i < VFPSystemRegister::VFP_SYSTEM_REGISTER_COUNT; i++) { - auto r = GetVFPSystemReg(static_cast<VFPSystemRegister>(i)); + for (size_t i = 0; i < VFPSystemRegister::VFP_SYSTEM_REGISTER_COUNT; i++) { + const auto r = GetVFPSystemReg(static_cast<VFPSystemRegister>(i)); ar << r; } - for (auto i = 0; i < CP15Register::CP15_REGISTER_COUNT; i++) { - auto r = GetCP15Register(static_cast<CP15Register>(i)); + for (size_t i = 0; i < CP15Register::CP15_REGISTER_COUNT; i++) { + const auto r = GetCP15Register(static_cast<CP15Register>(i)); ar << r; } } @@ -274,11 +274,11 @@ private: PurgeState(); ar >> timer; ar >> id; - std::shared_ptr<Memory::PageTable> page_table = nullptr; + std::shared_ptr<Memory::PageTable> page_table{}; ar >> page_table; SetPageTable(page_table); u32 r; - for (size_t = 0; i < 15; i++) { + for (int i = 0; i < 15; i++) { ar >> r; SetReg(i, r); } @@ -286,15 +286,15 @@ private: SetPC(r); ar >> r; SetCPSR(r); - for (auto i = 0; i < 32; i++) { + for (int i = 0; i < 32; i++) { ar >> r; SetVFPReg(i, r); } - for (auto i = 0; i < VFPSystemRegister::VFP_SYSTEM_REGISTER_COUNT; i++) { + for (size_t i = 0; i < VFPSystemRegister::VFP_SYSTEM_REGISTER_COUNT; i++) { ar >> r; SetVFPSystemReg(static_cast<VFPSystemRegister>(i), r); } - for (auto i = 0; i < CP15Register::CP15_REGISTER_COUNT; i++) { + for (size_t i = 0; i < CP15Register::CP15_REGISTER_COUNT; i++) { ar >> r; SetCP15Register(static_cast<CP15Register>(i), r); } diff --git a/src/core/hle/kernel/config_mem.h b/src/core/hle/kernel/config_mem.h index a81a5d291..b592aff7a 100644 --- a/src/core/hle/kernel/config_mem.h +++ b/src/core/hle/kernel/config_mem.h @@ -58,10 +58,14 @@ public: ConfigMemDef& GetConfigMem(); u8* GetPtr() override { - return reinterpret_cast<u8*>(&config_mem)); + return reinterpret_cast<u8*>(&config_mem); } - u32 GetSize() const override { + const u8* GetPtr() const override { + return reinterpret_cast<const u8*>(&config_mem); + } + + std::size_t GetSize() const override { return sizeof(config_mem); } diff --git a/src/core/hle/kernel/kernel.cpp b/src/core/hle/kernel/kernel.cpp index 11ef90771..f0974f88a 100644 --- a/src/core/hle/kernel/kernel.cpp +++ b/src/core/hle/kernel/kernel.cpp @@ -24,10 +24,8 @@ KernelSystem::KernelSystem(Memory::MemorySystem& memory, Core::Timing& timing, u32 num_cores, u8 n3ds_mode) : memory(memory), timing(timing), prepare_reschedule_callback(std::move(prepare_reschedule_callback)) { -std::generate(memory_regions.begin(), memory_regions.end(), - [] { return std::make_shared<MemoryRegionInfo>(); }); - memory_regions[i] = std::make_shared<MemoryRegionInfo>(); - } + std::generate(memory_regions.begin(), memory_regions.end(), + [] { return std::make_shared<MemoryRegionInfo>(); }); MemoryInit(system_mode, n3ds_mode); resource_limits = std::make_unique<ResourceLimitList>(*this); diff --git a/src/core/hle/kernel/shared_page.h b/src/core/hle/kernel/shared_page.h index b74470b4a..c4b174db4 100644 --- a/src/core/hle/kernel/shared_page.h +++ b/src/core/hle/kernel/shared_page.h @@ -99,11 +99,15 @@ public: SharedPageDef& GetSharedPage(); - virtual u8* GetPtr() { - return static_cast<u8*>(static_cast<void*>(&shared_page)); + u8* GetPtr() override { + return reinterpret_cast<u8*>(&shared_page); } - virtual u32 GetSize() const { + const u8* GetPtr() const override { + return reinterpret_cast<const u8*>(&shared_page); + } + + std::size_t GetSize() const override { return sizeof(shared_page); } diff --git a/src/core/hle/service/cecd/cecd.cpp b/src/core/hle/service/cecd/cecd.cpp index 36263f622..8db49fc82 100644 --- a/src/core/hle/service/cecd/cecd.cpp +++ b/src/core/hle/service/cecd/cecd.cpp @@ -106,7 +106,7 @@ void Module::Interface::Open(Kernel::HLERequestContext& ctx) { } else { session_data->file = std::move(file_result).Unwrap(); rb.Push(RESULT_SUCCESS); - rb.Push<u32>(session_data->file->GetSize()); // Return file size + rb.Push<u32>(static_cast<u32>(session_data->file->GetSize())); // Return file size } if (path_type == CecDataPathType::MboxProgramId) { @@ -154,8 +154,8 @@ void Module::Interface::Read(Kernel::HLERequestContext& ctx) { break; default: // If not directory, then it is a file std::vector<u8> buffer(write_buffer_size); - const u32 bytes_read = - session_data->file->Read(0, write_buffer_size, buffer.data()).Unwrap(); + const u32 bytes_read = static_cast<u32>( + session_data->file->Read(0, write_buffer_size, buffer.data()).Unwrap()); write_buffer.Write(buffer.data(), 0, write_buffer_size); session_data->file->Close(); @@ -197,7 +197,8 @@ void Module::Interface::ReadMessage(Kernel::HLERequestContext& ctx) { auto message = std::move(message_result).Unwrap(); std::vector<u8> buffer(buffer_size); - const u32 bytes_read = message->Read(0, buffer_size, buffer.data()).Unwrap(); + const u32 bytes_read = + static_cast<u32>(message->Read(0, buffer_size, buffer.data()).Unwrap()); write_buffer.Write(buffer.data(), 0, buffer_size); message->Close(); @@ -266,7 +267,8 @@ void Module::Interface::ReadMessageWithHMAC(Kernel::HLERequestContext& ctx) { auto message = std::move(message_result).Unwrap(); std::vector<u8> buffer(buffer_size); - const u32 bytes_read = message->Read(0, buffer_size, buffer.data()).Unwrap(); + const u32 bytes_read = + static_cast<u32>(message->Read(0, buffer_size, buffer.data()).Unwrap()); write_buffer.Write(buffer.data(), 0, buffer_size); message->Close(); @@ -367,8 +369,8 @@ void Module::Interface::Write(Kernel::HLERequestContext& ctx) { buffer); } - const u32 bytes_written = - session_data->file->Write(0, buffer.size(), true, buffer.data()).Unwrap(); + const u32 bytes_written = static_cast<u32>( + session_data->file->Write(0, buffer.size(), true, buffer.data()).Unwrap()); session_data->file->Close(); rb.Push(RESULT_SUCCESS); @@ -429,7 +431,8 @@ void Module::Interface::WriteMessage(Kernel::HLERequestContext& ctx) { msg_header.sender_id, msg_header.sender_id2, msg_header.send_count, msg_header.forward_count, msg_header.user_data); - const u32 bytes_written = message->Write(0, buffer_size, true, buffer.data()).Unwrap(); + const u32 bytes_written = + static_cast<u32>(message->Write(0, buffer_size, true, buffer.data()).Unwrap()); message->Close(); rb.Push(RESULT_SUCCESS); @@ -515,7 +518,8 @@ void Module::Interface::WriteMessageWithHMAC(Kernel::HLERequestContext& ctx) { hmac.CalculateDigest(hmac_digest.data(), message_body.data(), msg_header.body_size); std::memcpy(buffer.data() + hmac_offset, hmac_digest.data(), hmac_size); - const u32 bytes_written = message->Write(0, buffer_size, true, buffer.data()).Unwrap(); + const u32 bytes_written = + static_cast<u32>(message->Write(0, buffer_size, true, buffer.data()).Unwrap()); message->Close(); rb.Push(RESULT_SUCCESS); @@ -757,7 +761,8 @@ void Module::Interface::OpenAndWrite(Kernel::HLERequestContext& ctx) { cecd->CheckAndUpdateFile(path_type, ncch_program_id, buffer); } - const u32 bytes_written = file->Write(0, buffer.size(), true, buffer.data()).Unwrap(); + const u32 bytes_written = + static_cast<u32>(file->Write(0, buffer.size(), true, buffer.data()).Unwrap()); file->Close(); rb.Push(RESULT_SUCCESS); @@ -806,7 +811,8 @@ void Module::Interface::OpenAndRead(Kernel::HLERequestContext& ctx) { auto file = std::move(file_result).Unwrap(); std::vector<u8> buffer(buffer_size); - const u32 bytes_read = file->Read(0, buffer_size, buffer.data()).Unwrap(); + const u32 bytes_read = + static_cast<u32>(file->Read(0, buffer_size, buffer.data()).Unwrap()); write_buffer.Write(buffer.data(), 0, buffer_size); file->Close(); @@ -937,7 +943,7 @@ void Module::CheckAndUpdateFile(const CecDataPathType path_type, const u32 ncch_ constexpr u32 max_num_boxes = 24; constexpr u32 name_size = 16; // fixed size 16 characters long constexpr u32 valid_name_size = 8; // 8 characters are valid, the rest are null - const u32 file_size = file_buffer.size(); + const u32 file_size = static_cast<u32>(file_buffer.size()); switch (path_type) { case CecDataPathType::MboxList: { @@ -1021,7 +1027,7 @@ void Module::CheckAndUpdateFile(const CecDataPathType path_type, const u32 ncch_ std::u16string u16_filename; // Loop through entries but don't add mboxlist____ to itself. - for (auto i = 0; i < entry_count; i++) { + for (u32 i = 0; i < entry_count; i++) { u16_filename = std::u16string(entries[i].filename); file_name = Common::UTF16ToUTF8(u16_filename); @@ -1212,7 +1218,7 @@ void Module::CheckAndUpdateFile(const CecDataPathType path_type, const u32 ncch_ std::string file_name; std::u16string u16_filename; - for (auto i = 0; i < entry_count; i++) { + for (u32 i = 0; i < entry_count; i++) { u16_filename = std::u16string(entries[i].filename); file_name = Common::UTF16ToUTF8(u16_filename); @@ -1230,7 +1236,7 @@ void Module::CheckAndUpdateFile(const CecDataPathType path_type, const u32 ncch_ auto message_result = cecd_system_save_data_archive->OpenFile(message_path, mode); auto message = std::move(message_result).Unwrap(); - const u32 message_size = message->GetSize(); + const u32 message_size = static_cast<u32>(message->GetSize()); std::vector<u8> buffer(message_size); message->Read(0, message_size, buffer.data()).Unwrap(); @@ -1304,7 +1310,7 @@ void Module::CheckAndUpdateFile(const CecDataPathType path_type, const u32 ncch_ std::string file_name; std::u16string u16_filename; - for (auto i = 0; i < entry_count; i++) { + for (u32 i = 0; i < entry_count; i++) { u16_filename = std::u16string(entries[i].filename); file_name = Common::UTF16ToUTF8(u16_filename); @@ -1320,7 +1326,7 @@ void Module::CheckAndUpdateFile(const CecDataPathType path_type, const u32 ncch_ auto message_result = cecd_system_save_data_archive->OpenFile(message_path, mode); auto message = std::move(message_result).Unwrap(); - const u32 message_size = message->GetSize(); + const u32 message_size = static_cast<u32>(message->GetSize()); std::vector<u8> buffer(message_size); message->Read(0, message_size, buffer.data()).Unwrap(); @@ -1353,7 +1359,8 @@ void Module::CheckAndUpdateFile(const CecDataPathType path_type, const u32 ncch_ case CecDataPathType::MboxData: case CecDataPathType::MboxIcon: case CecDataPathType::MboxTitle: - default: {} + default: { + } } } diff --git a/src/core/hle/service/http_c.cpp b/src/core/hle/service/http_c.cpp index af639e493..4879ab371 100644 --- a/src/core/hle/service/http_c.cpp +++ b/src/core/hle/service/http_c.cpp @@ -83,10 +83,10 @@ void Context::MakeRequest() { client = std::move(ssl_client); if (auto client_cert = ssl_config.client_cert_ctx.lock()) { - SSL_CTX_use_certificate_ASN1(ctx, client_cert->certificate.size(), + SSL_CTX_use_certificate_ASN1(ctx, static_cast<int>(client_cert->certificate.size()), client_cert->certificate.data()); SSL_CTX_use_PrivateKey_ASN1(EVP_PKEY_RSA, ctx, client_cert->private_key.data(), - client_cert->private_key.size()); + static_cast<long>(client_cert->private_key.size())); } // TODO(B3N30): Check for SSLOptions-Bits and set the verify method accordingly diff --git a/src/core/memory.cpp b/src/core/memory.cpp index a0fa2a738..c0c030de1 100644 --- a/src/core/memory.cpp +++ b/src/core/memory.cpp @@ -100,7 +100,7 @@ public: Impl(); - virtual u8* GetPtr(Region r) { + const u8* GetPtr(Region r) const { switch (r) { case Region::VRAM: return vram.get(); @@ -115,7 +115,22 @@ public: } } - virtual u32 GetSize(Region r) const { + u8* GetPtr(Region r) { + switch (r) { + case Region::VRAM: + return vram.get(); + case Region::DSP: + return dsp->GetDspMemory().data(); + case Region::FCRAM: + return fcram.get(); + case Region::N3DS: + return n3ds_extra_ram.get(); + default: + UNREACHABLE(); + } + } + + u32 GetSize(Region r) const { switch (r) { case Region::VRAM: return VRAM_SIZE; @@ -158,11 +173,14 @@ template <Region R> class MemorySystem::BackingMemImpl : public BackingMem { public: BackingMemImpl() : impl(*Core::Global<Core::System>().Memory().impl) {} - BackingMemImpl(MemorySystem::Impl& impl_) : impl(impl_) {} - virtual u8* GetPtr() { + explicit BackingMemImpl(MemorySystem::Impl& impl_) : impl(impl_) {} + u8* GetPtr() override { return impl.GetPtr(R); } - virtual u32 GetSize() const { + const u8* GetPtr() const override { + return impl.GetPtr(R); + } + std::size_t GetSize() const override { return impl.GetSize(R); } @@ -884,7 +902,7 @@ void WriteMMIO<u64>(MMIORegionPointer mmio_handler, VAddr addr, const u64 data) u32 MemorySystem::GetFCRAMOffset(const u8* pointer) { ASSERT(pointer >= impl->fcram.get() && pointer <= impl->fcram.get() + Memory::FCRAM_N3DS_SIZE); - return pointer - impl->fcram.get(); + return static_cast<u32>(pointer - impl->fcram.get()); } u8* MemorySystem::GetFCRAMPointer(u32 offset) {