From f1948d50544c36857047c12e46f10a3a1d41160d Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Wed, 19 Aug 2026 21:08:42 -0400 Subject: [PATCH] terminal: transfer selection text into string maps Selection strings previously duplicated formatted text when callers requested a StringMap, even though the regex-link caller immediately freed the returned copy. Add a dedicated selectionStringMap path that transfers the formatter output and pin map directly into the returned map. This removes the extra allocation and copy while making ownership explicit. --- src/Surface.zig | 6 +-- src/terminal/Screen.zig | 93 ++++++++++++-------------------------- src/terminal/StringMap.zig | 15 ++---- 3 files changed, 34 insertions(+), 80 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index fcf224b0a..c57eeab54 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -4369,12 +4369,10 @@ fn linkAtPin( .semantic_prompt_boundary = true, }) orelse return null; - var strmap: terminal.StringMap = undefined; - self.alloc.free(try screen.selectionString(self.alloc, .{ + const strmap = try screen.selectionStringMap(self.alloc, .{ .sel = line, .trim = false, - .map = &strmap, - })); + }); defer strmap.deinit(self.alloc); for (self.config.links) |link| { diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 41149f6da..29305cbc6 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -2872,17 +2872,8 @@ pub const SelectionString = struct { /// If true, trim whitespace around the selection. trim: bool = true, - - /// If non-null, a stringmap will be written here. This will use - /// the same allocator as the call to selectionString. The string will - /// be duplicated here and in the return value so both must be freed. - map: ?*StringMap = null, }; -const selectionString_tw = tripwire.module(enum { - copy_map, -}, selectionString); - /// Returns the raw text associated with a selection. This will unwrap /// soft-wrapped edges. The returned slice is owned by the caller and allocated /// using alloc, not the allocator associated with the screen (unless they match). @@ -2892,6 +2883,32 @@ pub fn selectionString( self: *Screen, alloc: Allocator, opts: SelectionString, +) Allocator.Error![:0]const u8 { + return self.selectionStringImpl(alloc, opts, null); +} + +/// Returns a StringMap associated with a selection. The map contains the +/// selection's raw text and a mapping from each byte to its screen location. +/// +/// The returned map is owned by the caller. +pub fn selectionStringMap( + self: *Screen, + alloc: Allocator, + opts: SelectionString, +) Allocator.Error!StringMap { + var pins: PinMap.Map = .empty; + errdefer pins.deinit(alloc); + return .{ + .string = try self.selectionStringImpl(alloc, opts, &pins), + .map = pins, + }; +} + +fn selectionStringImpl( + self: *Screen, + alloc: Allocator, + opts: SelectionString, + pins: ?*PinMap.Map, ) Allocator.Error![:0]const u8 { // We'll use this as our buffer to build our string. var aw: std.Io.Writer.Allocating = .init(alloc); @@ -2908,35 +2925,15 @@ pub fn selectionString( ); formatter.content = .{ .selection = opts.sel }; - // If we have a string map, we need to set that up. - var pins: PinMap.Map = .empty; - defer pins.deinit(alloc); - if (opts.map != null) formatter.pin_map = .{ + if (pins) |map| formatter.pin_map = .{ .alloc = alloc, - .map = &pins, + .map = map, }; // Emit. Since this is an allocating writer, a failed write // just becomes an OOM. formatter.format(&aw.writer) catch return error.OutOfMemory; - - // Build our final text and if we have a string map set that up. - const text = try aw.toOwnedSliceSentinel(0); - errdefer alloc.free(text); - if (opts.map) |map| { - const map_string = try alloc.dupeZ(u8, text); - errdefer alloc.free(map_string); - try selectionString_tw.check(.copy_map); - map.* = .{ - .string = map_string, - .map = pins, - }; - - // Ownership of the pin map moved to the string map. - pins = .empty; - } - - return text; + return try aw.toOwnedSliceSentinel(0); } pub const SelectLine = struct { @@ -11440,38 +11437,6 @@ test "Screen setAttribute splits page on OutOfSpace at max styles" { try testing.expect(page_was_split); } -test "selectionString map allocation failure cleanup" { - // This test verifies that if toOwnedSlice fails when building - // the StringMap, we don't leak the already-allocated map.string. - const testing = std.testing; - const alloc = testing.allocator; - const io = testing.io; - var s = try Screen.init(io, alloc, .{ .cols = 10, .rows = 5, .max_scrollback_bytes = 0 }); - defer s.deinit(); - - try s.testWriteString("hello"); - - // Get a selection - const sel = Selection.init( - s.pages.pin(.{ .active = .{ .x = 0, .y = 0 } }).?, - s.pages.pin(.{ .active = .{ .x = 4, .y = 0 } }).?, - false, - ); - - // Trigger allocation failure on toOwnedSlice - var map: StringMap = undefined; - selectionString_tw.errorAlways(.copy_map, error.OutOfMemory); - const result = s.selectionString(alloc, .{ - .sel = sel, - .map = &map, - }); - try testing.expectError(error.OutOfMemory, result); - try selectionString_tw.end(.reset); - - // If this test passes without memory leaks (when run with testing.allocator), - // it means the errdefer properly cleaned up map.string when toOwnedSlice failed. -} - test "Screen: promptClickMove line right basic" { const testing = std.testing; const alloc = testing.allocator; diff --git a/src/terminal/StringMap.zig b/src/terminal/StringMap.zig index b06a6d176..7b8aac067 100644 --- a/src/terminal/StringMap.zig +++ b/src/terminal/StringMap.zig @@ -145,13 +145,10 @@ test "StringMap searchIterator" { .y = 1, } }).?, }).?; - var map: StringMap = undefined; - const sel_str = try s.selectionString(alloc, .{ + const map = try s.selectionStringMap(alloc, .{ .sel = line, .trim = false, - .map = &map, }); - alloc.free(sel_str); defer map.deinit(alloc); // Get our iterator @@ -205,13 +202,10 @@ test "StringMap searchIterator URL detection" { .y = 0, } }).?, }).?; - var map: StringMap = undefined; - const sel_str = try s.selectionString(alloc, .{ + const map = try s.selectionStringMap(alloc, .{ .sel = line, .trim = false, - .map = &map, }); - alloc.free(sel_str); defer map.deinit(alloc); // Search for URL match @@ -270,13 +264,10 @@ test "StringMap searchIterator URL with click position" { const line = s.selectLine(.{ .pin = click_pin, }).?; - var map: StringMap = undefined; - const sel_str = try s.selectionString(alloc, .{ + const map = try s.selectionStringMap(alloc, .{ .sel = line, .trim = false, - .map = &map, }); - alloc.free(sel_str); defer map.deinit(alloc); // Search for URL match and verify click position is within URL