Value semantics

Explain
1

I have code that looks like this

pub const Gameboy = struct {
    mbc: cartridge.MBC,
    cpu: cpu.CPU,
    gpu: gpu.GPU,
    memory_bus: memory_bus.MemoryBus,
    joypad: joypad.Joypad,
    timer: timer.Timer,

    pub fn new(filename: []u8) !Gameboy {
        var mbc_ = try cartridge.MBC.new(filename);
        var gpu_ = gpu.GPU.new();
        var joypad_ = joypad.Joypad.new();
        var timer_ = timer.Timer.new();
        timer_.tac.frequency = timer.Frequency.Hz4096;
        var memory_bus_ = try memory_bus.MemoryBus.new(&mbc_, &gpu_, &timer_, &joypad_);
        const cpu_ = try cpu.CPU.new(&memory_bus_);

        return Gameboy{
            .mbc = mbc_,
            .cpu = cpu_,
            .gpu = gpu_,
            .joypad = joypad_,
            .timer = timer_,
            .memory_bus = memory_bus_,
        };
    }

And am running into problems, which are making me question some assumptions I had about how things work.

is writing an initialization function like this even safe? ignore the reference usage for now. I’ve written a bunch of pass-by-value, return-by-value code that looks like a = A.new(B,C);, Where a stores B,C as members for the lifetime of the program. Are the variables safely copied/forwarded by reference when you do this?

The code above was after a long refactor attempt by me. Before, the ownership was A owns B, B owns X Y Z, all passed by value then stored as members. Perhaps now that I’m trying to do some shared ownership I’m bumping into issues?

Is there a way to do what I’m attempting without allocation?

2

Hi, if your new functions pass-by-value as you’ve said, then they will be either copied or forwarded by reference, yes. See this langref section.

You might want to check out this thread, too:

3

hi :smiley:

Passing a pointer to the local variable inside the Gameboy.new function to the MemoryBus.new is a bug if it stores the pointer somewhere and tries to use it later. The pointer becomes invalid when the Gameboy.new function returns. I would probably consider inlining the structs and having a single big struct that does not need to wire up pointers between it’s members to access the other variables (code does not need to mirror our model of the world, it’s often very inconvenient to do so).

Alternatively you can keep the sub-structs but don’t store the pointers inside the MemoryBus. Instead, take a gpu/timer/joypad pointer as a MemoryBus’ method argument when the method needs it.

It would be helpful to see how the other constructors look like to better answer your question.

Why can the other constructors fail? Do they do IO? Or do they grab a global allocator and allocate some things? If that is the case, it is a convention in Zig to pass a std.mem.Allocator as a function argument and use that.

Note that it is a convention to call the construction init and destructor deinit but there is ofc nothing wrong with new.

1 Like
5 
diff --git a/src/cartridge.zig b/src/cartridge.zig
index edc65d5..d565b9d 100644
--- a/src/cartridge.zig
+++ b/src/cartridge.zig
@@ -273,7 +273,6 @@ pub const MBC = struct {
     rom_size: RomSize,
     ram_size: RamSize,
 
-    gpa: std.heap.GeneralPurposeAllocator(.{}),
     allocator: std.mem.Allocator,
 
     pub fn handle_register(self: *MBC, address: u16, byte: u8) void {
@@ -569,16 +568,14 @@ pub const MBC = struct {
         self.banking_mode = mode & 0x01;
     }
 
-    pub fn new(filename: []u8) !MBC {
+    pub fn new(filename: []u8, alc: std.mem.Allocator) !MBC {
+        // It might be better to take a byte array with the rom already read
+        // instead of a filename to make the function easier to test and also
+        // move IO away
         const file = try std.fs.cwd().openFile(filename, .{});
         defer file.close();
 
-        const size = try file.getEndPos();
-
-        var gpa = std.heap.GeneralPurposeAllocator(.{}){};
-        const allocator = gpa.allocator();
-        const rom = try allocator.alloc(u8, size);
-        _ = try file.readAll(rom);
+        const rom = try file.readToEndAlloc(alc, std.math.maxInt(usize));
         std.debug.print("raw mbc {} rom size {}, ram size {}\n", .{
             rom[0x147],
             rom[0x148],
@@ -586,11 +583,16 @@ pub const MBC = struct {
         });
         const header = get_game_rom_metadata(rom);
 
-        const ram = try allocator.alloc(u8, header.ram_size.num_bytes());
+        // The maximum size this can be is 128KiB. It might be a good idea to
+        // allocate buffer of this size globally and slice it off here instead
+        // of doing an allocation. The downside is that you can only have a
+        // single cartridge instance.
+        const ram = try alc.alloc(u8, header.ram_size.num_bytes());
 
+        // Use std.log. std.debug.print is for printf-debugging
         std.debug.print("cartridge type: {}, size {}, rom size: {}, rom bytes: {}, rom banks: {}, ram size: {}\n", .{
             header.cartridge_type,
-            size,
+            rom.len,
             header.rom_size,
             header.rom_size.num_bytes(),
             header.rom_size.num_banks(),
@@ -610,8 +612,7 @@ pub const MBC = struct {
             .rom_size = header.rom_size,
             .ram_size = header.ram_size,
 
-            .gpa = gpa,
-            .allocator = allocator,
+            .allocator = alc,
         };
     }
 
@@ -619,7 +620,6 @@ pub const MBC = struct {
     pub fn deinit(self: *MBC) void {
         self.allocator.free(self.ram);
         self.allocator.free(self.rom);
-        self.gpa.deinit();
     }
 };
 pub fn get_game_rom_metadata(memory: []u8) GameBoyRomHeader {
diff --git a/src/cpu.zig b/src/cpu.zig
index c92fa2b..a503454 100644
--- a/src/cpu.zig
+++ b/src/cpu.zig
@@ -2604,7 +2604,7 @@ pub const CPU = struct {
         return address;
     }
 
-    pub fn new(bus: *MemoryBus) !CPU {
+    pub fn new(bus: *MemoryBus) CPU {
         const cpu: CPU = CPU{
             .registers = Registers{
                 .A = 0x01,
diff --git a/src/gameboy.zig b/src/gameboy.zig
index 2c50dd0..167a3d4 100644
--- a/src/gameboy.zig
+++ b/src/gameboy.zig
@@ -22,8 +22,8 @@ pub const Gameboy = struct {
         var joypad_ = joypad.Joypad.new();
         var timer_ = timer.Timer.new();
         timer_.tac.frequency = timer.Frequency.Hz4096;
-        var memory_bus_ = try memory_bus.MemoryBus.new(&mbc_, &gpu_, &timer_, &joypad_);
-        var cpu_ = try cpu.CPU.new(&memory_bus_);
+        var memory_bus_ = memory_bus.MemoryBus.new(&mbc_, &gpu_, &timer_, &joypad_);
+        var cpu_ = cpu.CPU.new(&memory_bus_);
 
         return Gameboy{
             .mbc = &mbc_,
diff --git a/src/memory_bus.zig b/src/memory_bus.zig
index 4c0dbd4..84529c3 100644
--- a/src/memory_bus.zig
+++ b/src/memory_bus.zig
@@ -81,7 +81,7 @@ pub const MemoryBus = struct {
     interrupt_enable: IERegister,
     interrupt_flag: IERegister,
 
-    pub fn new(mbc_: *MBC, gpu_: *GPU, timer_: *timer.Timer, joypad_: *joypad.Joypad) !MemoryBus {
+    pub fn new(mbc_: *MBC, gpu_: *GPU, timer_: *timer.Timer, joypad_: *joypad.Joypad) MemoryBus {
         var memory = [_]u8{0} ** 0x10000;
         std.mem.copyForwards(u8, memory[0..0x7FFF], mbc_.rom[cartridge.FULL_ROM_START..cartridge.FULL_ROM_END]);

making the cartridge constructor take already read file content instead of doing IO and using a static global memory buffer instead of allocating one dynamically would remove all error handling from the entire gameboy constructor, which i find neat.

6

ah yeah I had allocations in a couple extra places at one point. undecided on if i’ll fully move the allocation outside of cartridge

testing understanding… I can think of 3 options re the shared cpu/gameboy ownership of memorybus:

  • hoist all of these objects into main and pass them to gameboy (i think?), references wouldnt die because gb object lives forever
  • allocate memorybus locally to gameboy, pass that ref to cpu
  • hoist the memorybus code into gameboy and replace cpu.bus calls with fn ptrs passed in from gameboy

are all of those correct? any other options?