From b61fd5fbb6830b2546cfcaab0e17e6df010e4075 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 22 Jul 2026 13:48:10 -0700 Subject: [PATCH 1/5] terminal: add iterator to ref counted set --- src/terminal/ref_counted_set.zig | 87 ++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/src/terminal/ref_counted_set.zig b/src/terminal/ref_counted_set.zig index dd76079f4..97fa87088 100644 --- a/src/terminal/ref_counted_set.zig +++ b/src/terminal/ref_counted_set.zig @@ -1,5 +1,6 @@ const std = @import("std"); const assert = @import("../quirks.zig").inlineAssert; +const testing = std.testing; const size = @import("size.zig"); const Offset = size.Offset; @@ -447,6 +448,47 @@ pub fn RefCountedSet( return self.living; } + /// A live entry returned by `Iterator`. + pub const Entry = struct { + id: Id, + value_ptr: *T, + }; + + /// Iterates live entries in ascending ID order. + /// + /// Released entries whose reference count reached zero are skipped. + /// Any mutation of the set invalidates the iterator. + pub const Iterator = struct { + items: [*]Item, + id: Id = 1, + end: Id, + + pub fn next(self: *Iterator) ?Entry { + while (self.id < self.end) { + const id = self.id; + self.id += 1; + + const item = &self.items[id]; + if (item.meta.ref == 0) continue; + + return .{ + .id = id, + .value_ptr = &item.value, + }; + } + + return null; + } + }; + + /// Return an iterator over the live entries in this set. + pub fn iterator(self: *const Self, base: anytype) Iterator { + return .{ + .items = self.items.ptr(base), + .end = self.next_id, + }; + } + /// Delete an item, removing any references from /// the table, and freeing its ID to be reused. fn deleteItem(self: *Self, base: anytype, id: Id, ctx: Context) void { @@ -721,3 +763,48 @@ pub fn RefCountedSet( } }; } + +test "iterator visits live entries in ID order" { + const alloc = testing.allocator; + const TestSet = RefCountedSet( + u32, + u16, + u16, + struct { + pub fn hash(_: *const @This(), value: u32) u64 { + return std.hash.int(value); + } + + pub fn eql(_: *const @This(), a: u32, b: u32) bool { + return a == b; + } + }, + ); + + const layout: TestSet.Layout = .init(8); + const buf = try alloc.alignedAlloc( + u8, + TestSet.base_align, + layout.total_size, + ); + defer alloc.free(buf); + + var set: TestSet = .init(.init(buf), layout, .{}); + const first = try set.add(buf, 11); + const released = try set.add(buf, 22); + const last = try set.add(buf, 33); + _ = try set.add(buf, 11); + set.release(buf, released); + + var it = set.iterator(buf); + + const first_entry = it.next().?; + try testing.expectEqual(first, first_entry.id); + try testing.expectEqual(@as(u32, 11), first_entry.value_ptr.*); + + const last_entry = it.next().?; + try testing.expectEqual(last, last_entry.id); + try testing.expectEqual(@as(u32, 33), last_entry.value_ptr.*); + + try testing.expect(it.next() == null); +} From fc1bd06a1a090d0ebe59a6b04b021440fbae5b57 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 29 Jul 2026 14:04:11 -0700 Subject: [PATCH 2/5] terminal: PageList builder to build from raw pages --- src/terminal/PageList.zig | 289 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 289 insertions(+) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index e6fd48bb8..06a68a065 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -6806,6 +6806,295 @@ pub const Pin = struct { } }; +/// Build up a PageList manually from a set of Pages. +/// +/// This data structure is transactional: `deinit` releases every page +/// until `finish` is called. This keeps the ownership clear: a complete +/// PageList either owns all its pages or doesn't. +/// +/// This was specifically built to help facilitate snapshot decoding +/// which transfers pages directly, but could be generally useful +/// for other purposes as well. +pub const Builder = struct { + pool: MemoryPool, + pages: List = .{}, + page_serial: u64 = 0, + page_size: usize = 0, + options: Options, + + /// Initialize an empty builder. The options are the final state + /// of the PageList and some validation is done on the finish call + /// to ensure you built up a proper PageList according to those options. + pub fn init( + alloc: Allocator, + options: Options, + ) Allocator.Error!Builder { + return .{ + .pool = try MemoryPool.init( + alloc, + pageAllocator(), + page_preheat, + ), + .options = options, + }; + } + + /// Release all pages when restoration does not finish. This MUST NOT + /// be called when `finish` is successfully called. + pub fn deinit(self: *Builder) void { + // Free all our in-progress pages + while (self.pages.popFirst()) |node| destroyNodeExt( + &self.pool, + node, + &self.page_size, + ); + // Free memory pool + self.pool.deinit(); + self.* = undefined; + } + + /// Allocate a new page into the PageList with the given capacity. + /// + /// 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( + self: *Builder, + capacity: Capacity, + ) Allocator.Error!*Page { + const node = try createPageExt( + &self.pool, + .{ .cap = capacity }, + &self.page_serial, + &self.page_size, + ); + self.pages.append(node); + return node.pageAssumeResident(); + } + + pub const FinishError = Allocator.Error || error{ + InvalidDimensions, + InvalidPageDimensions, + NoPages, + InsufficientRows, + }; + + /// 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. + 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 + // sources into our PageList which asserts this. + if (self.options.cols == 0 or self.options.rows == 0) { + return error.InvalidDimensions; + } + if (self.pages.first == null) return error.NoPages; + + // Manually count our total rows at this point which we'll + // need for our PageList cache as well as a safety check. + const total_rows: usize = total_rows: { + var total_rows: usize = 0; + var node = self.pages.first; + while (node) |current| : (node = current.next) { + if (current.cols() == 0 or current.rows() == 0) { + return error.InvalidPageDimensions; + } + total_rows += current.rows(); + } + if (total_rows < self.options.rows) return error.InsufficientRows; + break :total_rows total_rows; + }; + + // Get our active pin + const active_top: Pin = active_top: { + var rem = self.options.rows; + var node = self.pages.last; + while (node) |current| : (node = current.prev) { + if (rem <= current.rows()) break :active_top .{ + .node = current, + .y = current.rows() - rem, + }; + rem -= current.rows(); + } else unreachable; + }; + + // Set our viewport up to the active + const viewport_pin = try self.pool.pins.create(); + errdefer self.pool.pins.destroy(viewport_pin); + viewport_pin.* = active_top; + + // Setup our one viewport tracked pin + var tracked_pins: PinSet = .{}; + errdefer tracked_pins.deinit(self.pool.alloc); + try tracked_pins.putNoClobber(self.pool.alloc, viewport_pin, {}); + + // Initialize limits + var limits: Limits = .init(self.options.cols, self.options.rows); + limits.set(.bytes, self.options.max_size); + limits.set(.lines, self.options.max_lines); + + const result: PageList = .{ + .cols = self.options.cols, + .rows = self.options.rows, + .pool = self.pool, + .pages = self.pages, + .page_serial = self.page_serial, + .page_serial_epoch = 0, + .page_size = self.page_size, + .limits = limits, + .total_rows = total_rows, + .tracked_pins = tracked_pins, + .viewport = .{ .active = {} }, + .viewport_pin = viewport_pin, + .viewport_pin_row_offset = null, + }; + result.assertIntegrity(); + self.* = undefined; + return result; + } +}; + +test "PageList Builder transfers mixed-width pages" { + const testing = std.testing; + + // Build two populated pages whose widths differ from each other and from + // the final active-area width. The first page also includes one row of + // incidental history above the three-row active area. + var result: PageList = result: { + var builder = try Builder.init(testing.allocator, .{ + .cols = 4, + .rows = 3, + .max_size = null, + .max_lines = null, + }); + errdefer builder.deinit(); + + const first = try builder.addPage(.{ + .cols = 2, + .rows = 2, + }); + first.size.rows = 2; + first.getRowAndCell(0, 0).cell.* = .init('A'); + + const second = try builder.addPage(.{ + .cols = 4, + .rows = 2, + }); + second.size.rows = 2; + second.getRowAndCell(0, 0).cell.* = .init('B'); + + break :result try builder.finish(); + }; + defer result.deinit(); + + // Successful finish transfers ownership and initializes the PageList's + // desired geometry, viewport, and required tracked viewport pin. + try testing.expectEqual(@as(size.CellCountInt, 4), result.cols); + try testing.expectEqual(@as(size.CellCountInt, 3), result.rows); + try testing.expectEqual(@as(usize, 2), result.totalPages()); + try testing.expectEqual(@as(usize, 1), result.countTrackedPins()); + try testing.expectEqual(Viewport.active, result.viewport); + + // Complete pages and their contents are preserved in insertion order, + // including widths which have not yet been reflowed. + const screen_top = result.getTopLeft(.screen); + try testing.expectEqual(@as(size.CellCountInt, 2), screen_top.node.cols()); + try testing.expectEqual(@as(u21, 'A'), screen_top + .node.page().getRowAndCell(0, 0).cell.codepoint()); + + // The active area is calculated backward from the newest page, so it + // begins at row one of the oldest page and leaves row zero as history. + const active_top = result.getTopLeft(.active); + try testing.expectEqual(screen_top.node, active_top.node); + try testing.expectEqual(@as(size.CellCountInt, 1), active_top.y); + try testing.expectEqual(@as(size.CellCountInt, 4), active_top + .node.next.?.cols()); + try testing.expectEqual(@as(u21, 'B'), active_top + .node.next.?.page().getRowAndCell(0, 0).cell.codepoint()); + + result.assertIntegrity(); +} + +test "PageList Builder validates the finished list" { + const testing = std.testing; + + // The desired PageList geometry must describe a non-empty screen. + { + var builder = try Builder.init(testing.allocator, .{ + .cols = 0, + .rows = 1, + }); + defer builder.deinit(); + try testing.expectError(error.InvalidDimensions, builder.finish()); + } + + // A PageList cannot be finished without any backing pages. + { + var builder = try Builder.init(testing.allocator, .{ + .cols = 1, + .rows = 1, + }); + defer builder.deinit(); + try testing.expectError(error.NoPages, builder.finish()); + } + + // Allocated capacity alone is insufficient: callers must populate a + // nonzero logical page size before transferring ownership. + { + var builder = try Builder.init(testing.allocator, .{ + .cols = 1, + .rows = 1, + }); + defer builder.deinit(); + const page = try builder.addPage(.{ .cols = 1, .rows = 1 }); + page.size.rows = 0; + try testing.expectError( + error.InvalidPageDimensions, + builder.finish(), + ); + } + + // The populated pages must contain enough rows to cover the active area. + { + var builder = try Builder.init(testing.allocator, .{ + .cols = 1, + .rows = 2, + }); + defer builder.deinit(); + const page = try builder.addPage(.{ .cols = 1, .rows = 1 }); + page.size.rows = 1; + try testing.expectError(error.InsufficientRows, builder.finish()); + } +} + +test "PageList Builder finish is transactional on allocation failure" { + const testing = std.testing; + + // Construct a valid builder so finish reaches its fallible bookkeeping + // allocations after all page and geometry validation succeeds. + var failing = testing.FailingAllocator.init(testing.allocator, .{}); + var builder = try Builder.init(failing.allocator(), .{ + .cols = 1, + .rows = 1, + }); + defer builder.deinit(); + const page = try builder.addPage(.{ .cols = 1, .rows = 1 }); + page.size.rows = 1; + + // The pools are preheated, so the next general allocation is the tracked + // viewport pin map created by finish. + failing.fail_index = failing.alloc_index; + try testing.expectError(error.OutOfMemory, builder.finish()); + try testing.expect(failing.has_induced_failure); + + // Failed finish leaves page ownership with the builder so its normal + // deinit path can release the still-linked page. + try testing.expect(builder.pages.first != null); + try testing.expectEqual(builder.pages.first, builder.pages.last); +} + fn mixedWidthPinListForTest(alloc: Allocator) !PageList { var result = try init(alloc, .{ .cols = 2, .rows = 1 }); errdefer result.deinit(); From 35db32078bc97a54334895bd32a83fce8fc0ef4c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 30 Jul 2026 07:21:05 -0700 Subject: [PATCH 3/5] terminal: PageList allocatePage --- src/terminal/PageList.zig | 331 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 331 insertions(+) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 06a68a065..71754af55 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -3843,6 +3843,123 @@ pub fn increaseCapacity( return new_node; } +/// Allocate a new page using the PageList's memory pools. +/// +/// 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 +/// throw it away. +pub fn allocatePage( + self: *PageList, + capacity: Capacity, +) Allocator.Error!PageAllocation { + return .{ + .destination = self, + .node = try createPageExt( + &self.pool, + .{ .cap = capacity }, + &self.page_serial, + null, + ), + }; +} + +/// One PageList-pooled page which has not yet joined the live page sequence. +pub const PageAllocation = struct { + destination: *PageList, + node: *List.Node, + + /// Return the fresh page storage for the caller to populate. + pub fn page(self: *PageAllocation) *Page { + return self.node.pageAssumeResident(); + } + + /// Release this uncommitted page back to its PageList's pools. + /// + /// This must not be called after `finalize` succeeds. + pub fn cancel(self: *PageAllocation) void { + destroyNodeExt( + &self.destination.pool, + self.node, + null, + ); + self.* = undefined; + } + + pub const Location = union(enum) { + /// Prepend the page to the start of the list (oldest history). + prepend, + }; + + /// Finalize this complete page and transfer its ownership to the PageList. + /// The parameter determines where it goes into the PageList. + /// + /// Existing pages and tracked pins keep their identity. A pinned viewport + /// keeps showing the same content while its cached absolute row offset + /// moves down by the number of newly inserted rows. + pub fn finalize(self: *PageAllocation, location: Location) FinalizeError!void { + switch (location) { + .prepend => return try self.prepend(), + } + } + + pub const FinalizeError = error{ + InvalidPageDimensions, + RowCountOverflow, + PageSizeOverflow, + MaxSizeExceeded, + MaxLinesExceeded, + }; + + fn prepend(self: *PageAllocation) FinalizeError!void { + const destination = self.destination; + const node = self.node; + + // Validate the populated page and all resulting accounting before + // publishing the detached node into the live list. + if (node.cols() == 0 or node.rows() == 0) return error.InvalidPageDimensions; + const total_rows = std.math.add( + usize, + destination.total_rows, + node.rows(), + ) catch return error.RowCountOverflow; + const node_size: usize = switch (node.owned) { + .pool => PagePool.item_size, + .heap => node.pageAssumeResident().memory.len, + }; + const page_size = std.math.add( + usize, + destination.page_size, + node_size, + ) catch return error.PageSizeOverflow; + + // Restored history is exact data, so reject a page which cannot + // coexist with the receiving PageList's configured limits. + if (page_size > destination.limits.max(.bytes)) { + return error.MaxSizeExceeded; + } + if (total_rows - destination.rows > destination.limits.max(.lines)) { + return error.MaxLinesExceeded; + } + + // No fallible work remains. Publish the page and update every cached + // quantity affected by inserting rows above the existing first page. + errdefer comptime unreachable; + destination.pages.prepend(node); + destination.page_size = page_size; + destination.total_rows = total_rows; + if (destination.viewport == .pin) { + if (destination.viewport_pin_row_offset) |*offset| { + offset.* += node.rows(); + } + } + destination.page_compression.markActivity(); + + destination.assertIntegrity(); + self.* = undefined; + } +}; + /// Options for createPage and createPageExt. const CreatePage = struct { /// The capacity to allocate the page with. @@ -7095,6 +7212,220 @@ test "PageList Builder finish is transactional on allocation failure" { try testing.expectEqual(builder.pages.first, builder.pages.last); } +test "PageList PageAllocation finalizes pages and preserves live state" { + const testing = std.testing; + + // Build an existing list with history, a two-row active area, an external + // active pin, and a pinned viewport whose absolute offset is cached. + var result: PageList = result: { + var builder = try Builder.init(testing.allocator, .{ + .cols = 4, + .rows = 2, + .max_size = null, + .max_lines = null, + }); + errdefer builder.deinit(); + + const first = try builder.addPage(.{ .cols = 3, .rows = 2 }); + first.size.rows = 2; + first.getRowAndCell(0, 0).cell.* = .init('C'); + + const second = try builder.addPage(.{ .cols = 4, .rows = 2 }); + second.size.rows = 2; + second.getRowAndCell(0, 0).cell.* = .init('D'); + + break :result try builder.finish(); + }; + defer result.deinit(); + + const old_first = result.pages.first.?; + const old_last = result.pages.last.?; + const active_top = result.getTopLeft(.active); + const tracked_active = try result.trackPin(active_top); + result.scroll(.{ .row = 1 }); + try testing.expectEqual(Viewport.pin, result.viewport); + try testing.expectEqual(@as(usize, 1), result.scrollbar().offset); + + // Prepend differently sized historical pages newest-first. Each page is + // populated while detached and joins the live list only on success. + { + var allocation = try result.allocatePage(.{ .cols = 4, .rows = 1 }); + errdefer allocation.cancel(); + const page = allocation.page(); + page.size.rows = 1; + page.getRowAndCell(0, 0).cell.* = .init('B'); + try allocation.finalize(.prepend); + } + { + var allocation = try result.allocatePage(.{ .cols = 2, .rows = 2 }); + errdefer allocation.cancel(); + const page = allocation.page(); + page.size.rows = 2; + page.getRowAndCell(0, 0).cell.* = .init('A'); + try allocation.finalize(.prepend); + } + + // Repeated prepends reconstruct oldest-to-newest order without replacing + // any existing nodes or tracked pins. + try testing.expectEqual(@as(usize, 4), result.totalPages()); + try testing.expectEqual(@as(usize, 7), result.total_rows); + try testing.expectEqual(old_last, result.pages.last.?); + try testing.expectEqual(old_first, result.pages.first.?.next.?.next.?); + try testing.expectEqual( + @as(u21, 'A'), + result.pages.first.?.page().getRowAndCell(0, 0).cell.codepoint(), + ); + try testing.expectEqual( + @as(u21, 'B'), + result.pages.first.?.next.? + .page().getRowAndCell(0, 0).cell.codepoint(), + ); + try testing.expect(active_top.eql(result.getTopLeft(.active))); + try testing.expect(active_top.eql(tracked_active.*)); + + // The viewport remains pinned to the same content, while its cached row + // offset and the scrollbar total include the three new historical rows. + try testing.expectEqual(Viewport.pin, result.viewport); + try testing.expectEqual(old_first, result.viewport_pin.node); + try testing.expectEqual(@as(size.CellCountInt, 1), result.viewport_pin.y); + const scrollbar_state = result.scrollbar(); + try testing.expectEqual(@as(usize, 7), scrollbar_state.total); + try testing.expectEqual(@as(usize, 4), scrollbar_state.offset); + try testing.expectEqual(@as(usize, 2), scrollbar_state.len); + + result.assertIntegrity(); +} + +test "PageList PageAllocation stays detached until finalize" { + const testing = std.testing; + + var result = try init(testing.allocator, .{ + .cols = 1, + .rows = 1, + .max_size = null, + .max_lines = null, + }); + defer result.deinit(); + + const initial_first = result.pages.first.?; + const initial_last = result.pages.last.?; + const initial_total_rows = result.total_rows; + 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. + var detached = try result.allocatePage(.{ .cols = 1, .rows = 1 }); + detached.page().size.rows = 1; + try testing.expectEqual(initial_first, result.pages.first.?); + try testing.expectEqual(initial_last, result.pages.last.?); + try testing.expectEqual(initial_total_rows, result.total_rows); + try testing.expectEqual(initial_page_size, result.page_size); + result.assertIntegrity(); + detached.cancel(); + 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(); + try testing.expectError( + error.InvalidPageDimensions, + invalid.finalize(.prepend), + ); + + try testing.expectEqual(initial_first, result.pages.first.?); + try testing.expectEqual(initial_last, result.pages.last.?); + try testing.expectEqual(initial_total_rows, result.total_rows); + try testing.expectEqual(initial_page_size, result.page_size); + result.assertIntegrity(); +} + +test "PageList PageAllocation rejects limits before modifying the destination" { + const testing = std.testing; + + var result = try init(testing.allocator, .{ + .cols = 1, + .rows = 1, + .max_size = 0, + .max_lines = null, + }); + defer result.deinit(); + + // The effective minimum permits one complete page beyond the active page. + // Fill that allowance so the following allocation exceeds the byte limit. + { + var allocation = try result.allocatePage(.{ .cols = 1, .rows = 1 }); + errdefer allocation.cancel(); + allocation.page().size.rows = 1; + try allocation.finalize(.prepend); + } + + const before_first = result.pages.first.?; + const before_total_rows = result.total_rows; + const before_page_size = result.page_size; + + var allocation = try result.allocatePage(.{ .cols = 1, .rows = 1 }); + defer allocation.cancel(); + allocation.page().size.rows = 1; + try testing.expectError( + error.MaxSizeExceeded, + allocation.finalize(.prepend), + ); + + try testing.expectEqual(before_first, result.pages.first.?); + try testing.expectEqual(before_total_rows, result.total_rows); + try testing.expectEqual(before_page_size, result.page_size); + result.assertIntegrity(); +} + +test "PageList PageAllocation allocation failure leaves list unchanged" { + const testing = std.testing; + + var failing = testing.FailingAllocator.init(testing.allocator, .{}); + var result = try init(failing.allocator(), .{ + .cols = 1, + .rows = 1, + .max_size = null, + .max_lines = null, + }); + defer result.deinit(); + + const initial_first = result.pages.first.?; + const initial_total_rows = result.total_rows; + const initial_page_size = result.page_size; + + // Existing pool capacity is deliberately an implementation detail. Allow + // preheated slots to succeed until allocation reaches node-pool growth. + failing.fail_index = failing.alloc_index; + var allocations: [64]PageAllocation = undefined; + var allocation_count: usize = 0; + defer for (allocations[0..allocation_count]) |*allocation| { + allocation.cancel(); + }; + + var failed = false; + for (0..64) |_| { + allocations[allocation_count] = result.allocatePage(.{ + .cols = 1, + .rows = 1, + }) catch |err| { + try testing.expectEqual(error.OutOfMemory, err); + failed = true; + break; + }; + allocation_count += 1; + } + try testing.expect(failed); + try testing.expect(failing.has_induced_failure); + + // Detached allocations and failed pool growth never publish into the live + // list; the deferred cleanup returns every successful allocation. + try testing.expectEqual(initial_first, result.pages.first.?); + try testing.expectEqual(initial_total_rows, result.total_rows); + try testing.expectEqual(initial_page_size, result.page_size); + result.assertIntegrity(); +} + fn mixedWidthPinListForTest(alloc: Allocator) !PageList { var result = try init(alloc, .{ .cols = 2, .rows = 1 }); errdefer result.deinit(); From e77c2612e51af6489cb60043af31aef2da3a5fa5 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 30 Jul 2026 08:51:07 -0700 Subject: [PATCH 4/5] lib: Zig enums have stable values --- src/lib/enum.zig | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/src/lib/enum.zig b/src/lib/enum.zig index 8a1bbb1fb..3c7a3a412 100644 --- a/src/lib/enum.zig +++ b/src/lib/enum.zig @@ -5,18 +5,16 @@ const Target = @import("target.zig").Target; /// if we're targeting C, otherwise a Zig enum with smallest possible /// backing type. /// -/// In all cases, the enum keys will be created in the order given. -/// For C ABI, this means that the order MUST NOT be changed in order -/// to preserve ABI compatibility. You can set a key to null to -/// remove it from the Zig enum while keeping the "hole" in the C enum -/// to preserve ABI compatibility. +/// In all cases, each enum value is its index in `keys`. The order MUST NOT +/// be changed when the integer values are part of an ABI or serialized format. +/// A null key removes that field while preserving its integer hole. /// /// C detection is up to the caller, since there are multiple ways /// to do that. We rely on the `target` parameter to determine whether we /// should create a C compatible enum or a Zig enum. /// -/// For the Zig enum, the enum value is not guaranteed to be stable, so -/// it shouldn't be relied for things like serialization. +/// C enums use `c_int`. Zig enums use the smallest unsigned integer that can +/// represent every key index, including holes. pub fn Enum( target: Target, keys: []const ?[:0]const u8, @@ -24,14 +22,12 @@ pub fn Enum( var names_raw: [keys.len][]const u8 = undefined; var values_raw: [keys.len]comptime_int = undefined; const names_actual, const values_actual = kv: { - // Remove any holes in the input (null values). As mentioned in the - // function docs, in the event of the C ABI, we need to preserve the - // original value, so we account for that. + // Remove null fields while preserving their positions as integer holes. var to: comptime_int = 0; for (0..keys.len) |from| { if (keys[from]) |key| { names_raw[to] = key; - values_raw[to] = if (target == .c) from else to; + values_raw[to] = from; to += 1; } } @@ -41,7 +37,7 @@ pub fn Enum( const TagInt = switch (target) { .c => c_int, - .zig => std.math.IntFittingRange(0, names_actual.len - 1), + .zig => std.math.IntFittingRange(0, keys.len - 1), }; return @Enum(TagInt, .exhaustive, names_actual, &(values: { @@ -69,7 +65,7 @@ test "c" { try testing.expectEqual(c_int, info.tag_type); } -test "abi by removing a key" { +test "stable values when removing a key" { const testing = std.testing; // C { @@ -84,10 +80,28 @@ test "abi by removing a key" { const T = Enum(.zig, &.{ "a", "b", null, "d" }); const info = @typeInfo(T).@"enum"; try testing.expectEqual(u2, info.tag_type); - try testing.expectEqual(2, @intFromEnum(T.d)); + try testing.expectEqual(3, @intFromEnum(T.d)); } } +test "zig backing integer includes trailing holes" { + const testing = std.testing; + const T = Enum(.zig, &.{ "a", null, null, null, null }); + const info = @typeInfo(T).@"enum"; + try testing.expectEqual(u3, info.tag_type); + try testing.expectEqual(0, @intFromEnum(T.a)); +} + +test "zig values remain stable across multiple holes" { + const testing = std.testing; + const T = Enum(.zig, &.{ null, "b", null, "d", null, "f" }); + const info = @typeInfo(T).@"enum"; + try testing.expectEqual(u3, info.tag_type); + try testing.expectEqual(1, @intFromEnum(T.b)); + try testing.expectEqual(3, @intFromEnum(T.d)); + try testing.expectEqual(5, @intFromEnum(T.f)); +} + /// Verify that for every key in enum T, there is a matching declaration in /// `ghostty.h` with the correct value. This should only ever be called inside a `test` /// because the `ghostty.h` module is only available then. From 457c5a0a64632282f7f8c2833013b38dc2c312ed Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 30 Jul 2026 11:02:12 -0700 Subject: [PATCH 5/5] 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;