Use shared_ptr and weak_ptr instead of unique_ptr and raw pointers
#673
Conversation
Codecov Report
@@ Coverage Diff @@
## master #673 +/- ##
==========================================
+ Coverage 94.05% 94.05% +<.01%
==========================================
Files 50 50
Lines 1665 1666 +1
==========================================
+ Hits 1566 1567 +1
Misses 99 99
Continue to review full report at Codecov.
|
simulator/memory/elf/elf_loader.cpp
Outdated
| @@ -9,10 +9,11 @@ | |||
| #include <elfio/elfio.hpp> | |||
| #include <memory/memory.h> | |||
|
|
|||
| static void load_elf_section( FuncMemory* memory, const ELFIO::section& section, AddrDiff offset) | |||
| static void load_elf_section( FuncMemory& memory, const ELFIO::section& section, AddrDiff offset) | |||
There was a problem hiding this comment.
We do not use non-const lvalue references as parameters, as it is not clear whether object is mutated.
|
Could you please describe where the circular dependencies occur in current code (w/o Kernel)? |
simulator/memory/elf/elf_loader.cpp
Outdated
| { | ||
| // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) Connecting ELFIO to our guidelines | ||
| memory->memcpy_host_to_guest( section.get_address() + offset, reinterpret_cast<const Byte*>(section.get_data()), section.get_size()); | ||
| memory.memcpy_host_to_guest( section.get_address() + offset, |
There was a problem hiding this comment.
NOLINTNEXTLINE is no longer effective, as reinterpret_cast is moved to the next line.
I think we need a separate statement to define a const Byte* pointer.
simulator/memory/elf/elf_loader.cpp
Outdated
| @@ -23,11 +24,11 @@ ElfLoader::ElfLoader( const std::string& filename, AddrDiff offset) | |||
| throw InvalidElfFile( filename); | |||
| } | |||
|
|
|||
| void ElfLoader::load_to( FuncMemory* memory) const | |||
| void ElfLoader::load_to( std::shared_ptr<FuncMemory> memory) const | |||
There was a problem hiding this comment.
My guideline would be to use a raw pointer here, as we are not going to own Memory inside ElfLoader.
There was a problem hiding this comment.
Alternatively, we may use const std::shared_ptr<FuncMemory>&. But reference to pointer is an awkward pattern...
As |
Yes, that's what I would propose. |
simulator/func_sim/instr_memory.h
Outdated
| @@ -14,10 +14,10 @@ | |||
| template<typename Instr> | |||
| class InstrMemory | |||
| { | |||
| FuncMemory* mem = nullptr; | |||
| std::weak_ptr<FuncMemory> mem; | |||
simulator/modules/mem/mem.h
Outdated
| @@ -21,7 +21,7 @@ class Mem : public Log | |||
| using InstructionOutput = std::pair< RegisterUInt, RegisterUInt>; | |||
|
|
|||
| private: | |||
| FuncMemory* memory = nullptr; | |||
| std::weak_ptr<FuncMemory> memory; | |||
#662
Circular dependencies are resolved by use of
weak_ptr's.