From 457c5a0a64632282f7f8c2833013b38dc2c312ed Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 30 Jul 2026 11:02:12 -0700 Subject: [PATCH] terminal: PageList align Builder/PageAllocation APIs better Rename Builder.addPage and PageAllocation.cancel to their consistent allocatePage and deinit forms. Track successful ownership transfers so both builder APIs can use unconditional deferred cleanup without releasing pages transferred to a PageList. --- src/terminal/PageList.zig | 74 +++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 71754af55..c9242aa81 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -3847,7 +3847,7 @@ pub fn increaseCapacity( /// /// The page is detached: it doesn't contribute to the memory limits or /// row counts or anything in the PageList. The caller must call `finalize` -/// to add it to the PageList at the appropriate place, or `cancel` to +/// to add it to the PageList at the appropriate place, or `deinit` to /// throw it away. pub fn allocatePage( self: *PageList, @@ -3867,23 +3867,25 @@ pub fn allocatePage( /// One PageList-pooled page which has not yet joined the live page sequence. pub const PageAllocation = struct { destination: *PageList, - node: *List.Node, + node: ?*List.Node, /// Return the fresh page storage for the caller to populate. pub fn page(self: *PageAllocation) *Page { - return self.node.pageAssumeResident(); + return self.node.?.pageAssumeResident(); } - /// Release this uncommitted page back to its PageList's pools. + /// Release an uncommitted page back to its PageList's pools. /// - /// This must not be called after `finalize` succeeds. - pub fn cancel(self: *PageAllocation) void { + /// This is safe to call after `finalize` succeeds, so callers can defer + /// it unconditionally. + pub fn deinit(self: *PageAllocation) void { + const node = self.node orelse return; destroyNodeExt( &self.destination.pool, - self.node, + node, null, ); - self.* = undefined; + self.node = null; } pub const Location = union(enum) { @@ -3913,7 +3915,7 @@ pub const PageAllocation = struct { fn prepend(self: *PageAllocation) FinalizeError!void { const destination = self.destination; - const node = self.node; + const node = self.node.?; // Validate the populated page and all resulting accounting before // publishing the detached node into the live list. @@ -3956,7 +3958,7 @@ pub const PageAllocation = struct { destination.page_compression.markActivity(); destination.assertIntegrity(); - self.* = undefined; + self.node = null; } }; @@ -6938,6 +6940,7 @@ pub const Builder = struct { page_serial: u64 = 0, page_size: usize = 0, options: Options, + finished: bool = false, /// Initialize an empty builder. The options are the final state /// of the PageList and some validation is done on the finish call @@ -6956,9 +6959,13 @@ pub const Builder = struct { }; } - /// Release all pages when restoration does not finish. This MUST NOT - /// be called when `finish` is successfully called. + /// Release all pages when restoration does not finish. + /// + /// This is safe to call after `finish` succeeds, so callers can defer it + /// unconditionally. pub fn deinit(self: *Builder) void { + if (self.finished) return; + // Free all our in-progress pages while (self.pages.popFirst()) |node| destroyNodeExt( &self.pool, @@ -6975,7 +6982,7 @@ pub const Builder = struct { /// The caller can then take this page and populate it. When `finish` /// is called, ownership is transferred to the resulting PageList. /// Until then, this Builder owns the page. - pub fn addPage( + pub fn allocatePage( self: *Builder, capacity: Capacity, ) Allocator.Error!*Page { @@ -6997,9 +7004,8 @@ pub const Builder = struct { }; /// Validate the decoded pages and transfer them into a live PageList. - /// This also frees all resources associated with the Builder (that - /// weren't transferred to the PageList). Do not call deinit after - /// this. + /// After this succeeds, `deinit` is a no-op because all resources have + /// transferred to the PageList. pub fn finish(self: *Builder) FinishError!PageList { // These are basic validations but they're cheap to do and // we want to be careful we don't let corruption from untrusted @@ -7068,7 +7074,7 @@ pub const Builder = struct { .viewport_pin_row_offset = null, }; result.assertIntegrity(); - self.* = undefined; + self.finished = true; return result; } }; @@ -7086,16 +7092,16 @@ test "PageList Builder transfers mixed-width pages" { .max_size = null, .max_lines = null, }); - errdefer builder.deinit(); + defer builder.deinit(); - const first = try builder.addPage(.{ + const first = try builder.allocatePage(.{ .cols = 2, .rows = 2, }); first.size.rows = 2; first.getRowAndCell(0, 0).cell.* = .init('A'); - const second = try builder.addPage(.{ + const second = try builder.allocatePage(.{ .cols = 4, .rows = 2, }); @@ -7165,7 +7171,7 @@ test "PageList Builder validates the finished list" { .rows = 1, }); defer builder.deinit(); - const page = try builder.addPage(.{ .cols = 1, .rows = 1 }); + const page = try builder.allocatePage(.{ .cols = 1, .rows = 1 }); page.size.rows = 0; try testing.expectError( error.InvalidPageDimensions, @@ -7180,7 +7186,7 @@ test "PageList Builder validates the finished list" { .rows = 2, }); defer builder.deinit(); - const page = try builder.addPage(.{ .cols = 1, .rows = 1 }); + const page = try builder.allocatePage(.{ .cols = 1, .rows = 1 }); page.size.rows = 1; try testing.expectError(error.InsufficientRows, builder.finish()); } @@ -7197,7 +7203,7 @@ test "PageList Builder finish is transactional on allocation failure" { .rows = 1, }); defer builder.deinit(); - const page = try builder.addPage(.{ .cols = 1, .rows = 1 }); + const page = try builder.allocatePage(.{ .cols = 1, .rows = 1 }); page.size.rows = 1; // The pools are preheated, so the next general allocation is the tracked @@ -7224,13 +7230,13 @@ test "PageList PageAllocation finalizes pages and preserves live state" { .max_size = null, .max_lines = null, }); - errdefer builder.deinit(); + defer builder.deinit(); - const first = try builder.addPage(.{ .cols = 3, .rows = 2 }); + const first = try builder.allocatePage(.{ .cols = 3, .rows = 2 }); first.size.rows = 2; first.getRowAndCell(0, 0).cell.* = .init('C'); - const second = try builder.addPage(.{ .cols = 4, .rows = 2 }); + const second = try builder.allocatePage(.{ .cols = 4, .rows = 2 }); second.size.rows = 2; second.getRowAndCell(0, 0).cell.* = .init('D'); @@ -7250,7 +7256,7 @@ test "PageList PageAllocation finalizes pages and preserves live state" { // populated while detached and joins the live list only on success. { var allocation = try result.allocatePage(.{ .cols = 4, .rows = 1 }); - errdefer allocation.cancel(); + defer allocation.deinit(); const page = allocation.page(); page.size.rows = 1; page.getRowAndCell(0, 0).cell.* = .init('B'); @@ -7258,7 +7264,7 @@ test "PageList PageAllocation finalizes pages and preserves live state" { } { var allocation = try result.allocatePage(.{ .cols = 2, .rows = 2 }); - errdefer allocation.cancel(); + defer allocation.deinit(); const page = allocation.page(); page.size.rows = 2; page.getRowAndCell(0, 0).cell.* = .init('A'); @@ -7313,7 +7319,7 @@ test "PageList PageAllocation stays detached until finalize" { const initial_page_size = result.page_size; // Allocating and populating a detached page does not alter any live list - // links or accounting. Cancel returns it to the same PageList pools. + // links or accounting. Deinit returns it to the same PageList pools. var detached = try result.allocatePage(.{ .cols = 1, .rows = 1 }); detached.page().size.rows = 1; try testing.expectEqual(initial_first, result.pages.first.?); @@ -7321,13 +7327,13 @@ test "PageList PageAllocation stays detached until finalize" { try testing.expectEqual(initial_total_rows, result.total_rows); try testing.expectEqual(initial_page_size, result.page_size); result.assertIntegrity(); - detached.cancel(); + detached.deinit(); result.assertIntegrity(); // Invalid populated dimensions leave ownership with the allocation so it // can still be released normally. var invalid = try result.allocatePage(.{ .cols = 1, .rows = 1 }); - defer invalid.cancel(); + defer invalid.deinit(); try testing.expectError( error.InvalidPageDimensions, invalid.finalize(.prepend), @@ -7355,7 +7361,7 @@ test "PageList PageAllocation rejects limits before modifying the destination" { // Fill that allowance so the following allocation exceeds the byte limit. { var allocation = try result.allocatePage(.{ .cols = 1, .rows = 1 }); - errdefer allocation.cancel(); + defer allocation.deinit(); allocation.page().size.rows = 1; try allocation.finalize(.prepend); } @@ -7365,7 +7371,7 @@ test "PageList PageAllocation rejects limits before modifying the destination" { const before_page_size = result.page_size; var allocation = try result.allocatePage(.{ .cols = 1, .rows = 1 }); - defer allocation.cancel(); + defer allocation.deinit(); allocation.page().size.rows = 1; try testing.expectError( error.MaxSizeExceeded, @@ -7400,7 +7406,7 @@ test "PageList PageAllocation allocation failure leaves list unchanged" { var allocations: [64]PageAllocation = undefined; var allocation_count: usize = 0; defer for (allocations[0..allocation_count]) |*allocation| { - allocation.cancel(); + allocation.deinit(); }; var failed = false;