fix: add compatibility for Zig 0.16 ArrayList and timer changes#23
fix: add compatibility for Zig 0.16 ArrayList and timer changes#23jinzhongjia merged 7 commits intomainfrom
Conversation
- Replace legacy ArrayList initializers with `initCapacity()` in relevant places to support Zig 0.16, preserving compatibility with older releases. - Update benchmarks to use platform time routines when std.time.Timer is removed in Zig 0.16, and switch to page_allocator where GeneralPurposeAllocator is no longer available. - Expand workflow support to include Zig 0.16.0. - Clarify version support and adaptation details in documentation.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Zig 0.16.0, updating version compatibility documentation and adapting the codebase to API changes in ArrayList initialization, timing, and memory allocation. Feedback focuses on correcting the false claim that GeneralPurposeAllocator was removed, suggesting the use of std.time.Instant for cross-platform timing, and recommending more idiomatic ArrayList initialization for Zig 0.16. Additionally, the reviewer advises against using page_allocator in benchmarks to avoid performance regressions.
| - ArrayList initialization changes in Zig 0.16 (`std.ArrayList(T){}` removed, use `initCapacity`) | ||
| - Build system API differences between versions | ||
| - `std.time.Timer` removed in 0.16 (replaced with platform-specific timing in benchmarks) | ||
| - `std.heap.GeneralPurposeAllocator` removed in 0.16 (replaced with `page_allocator`) |
There was a problem hiding this comment.
The claim that std.heap.GeneralPurposeAllocator is removed in Zig 0.16 is incorrect. It remains a core part of the standard library, although its internal implementation details or its location within std.heap may have evolved. Switching to page_allocator in benchmarks is a regression in measurement quality as it is significantly slower due to per-allocation system calls.
| fn getTimeNs() u64 { | ||
| if (builtin.os.tag == .windows) { | ||
| const w = std.os.windows; | ||
| var counter: w.LARGE_INTEGER = undefined; | ||
| _ = w.ntdll.RtlQueryPerformanceCounter(&counter); | ||
| var freq: w.LARGE_INTEGER = undefined; | ||
| _ = w.ntdll.RtlQueryPerformanceFrequency(&freq); | ||
| const ns = std.time.ns_per_s; | ||
| const c: u64 = @bitCast(counter); | ||
| const f: u64 = @bitCast(freq); | ||
| return @intCast(@divTrunc(@as(u128, c) * ns, @as(u128, f))); | ||
| } else if (builtin.os.tag != .windows) { | ||
| var ts: std.c.timespec = undefined; | ||
| _ = std.c.clock_gettime(std.c.CLOCK.MONOTONIC, &ts); | ||
| return @as(u64, @intCast(ts.tv_sec)) * std.time.ns_per_s + @as(u64, @intCast(ts.tv_nsec)); | ||
| } else @compileError("unsupported OS for benchmark"); | ||
| } |
There was a problem hiding this comment.
The manual implementation of getTimeNs introduces significant boilerplate and potential portability issues (e.g., dependency on libc for clock_gettime on non-Windows platforms). For Zig 0.16, the recommended replacement for std.time.Timer is std.time.Instant, which provides a cross-platform monotonic clock API. Additionally, the Windows implementation lacks a check for a zero frequency from RtlQueryPerformanceFrequency, which would cause a division by zero.
| } else if (builtin.os.tag != .windows) { | ||
| var ts: std.c.timespec = undefined; | ||
| _ = std.c.clock_gettime(std.c.CLOCK.MONOTONIC, &ts); | ||
| return @as(u64, @intCast(ts.tv_sec)) * std.time.ns_per_s + @as(u64, @intCast(ts.tv_nsec)); | ||
| } else @compileError("unsupported OS for benchmark"); |
There was a problem hiding this comment.
The else if (builtin.os.tag != .windows) condition is redundant following the initial if (builtin.os.tag == .windows). This makes the final else block containing the @compileError unreachable. It should be simplified to a standard else block.
} else {
var ts: std.c.timespec = undefined;
_ = std.c.clock_gettime(std.c.CLOCK.MONOTONIC, &ts);
return @as(u64, @intCast(ts.tv_sec)) * std.time.ns_per_s + @as(u64, @intCast(ts.tv_nsec));
}
| var gpa = std.heap.GeneralPurposeAllocator(.{}){}; | ||
| defer _ = gpa.deinit(); | ||
| const allocator = gpa.allocator(); | ||
| const allocator = std.heap.page_allocator; |
There was a problem hiding this comment.
Using std.heap.page_allocator for benchmarks is generally discouraged because it performs a system call for every allocation and deallocation, which can dominate the benchmark results and hide the actual performance characteristics of the code being tested. Since GeneralPurposeAllocator is still available in Zig 0.16, it should be used instead.
| var legacy_timer: if (is_zig_16) void else std.time.Timer = if (is_zig_16) {} else undefined; | ||
| if (!is_zig_16) legacy_timer = try std.time.Timer.start(); | ||
| const start_ns = if (is_zig_16) getTimeNs() else 0; |
There was a problem hiding this comment.
The conditional logic for handling the timer across Zig versions is quite clunky. Instead of manually implementing getTimeNs and using a void type for legacy_timer, it is recommended to use std.time.Instant for Zig 0.16. This would allow for a cleaner abstraction or a simple wrapper that works across all supported versions.
| var new_heap = if (current_zig.minor == 14) | ||
| std.ArrayList(Payload).init(alloc) | ||
| else | ||
| std.ArrayList(Payload){}; | ||
| std.ArrayList(Payload).initCapacity(alloc, 0) catch unreachable; |
There was a problem hiding this comment.
Using initCapacity(alloc, 0) catch unreachable is suboptimal and introduces a potential crash point during memory cleanup. In Zig 0.16, std.ArrayList(T).init(allocator) is the preferred way to initialize a managed ArrayList as it does not perform an initial allocation and thus cannot fail. The logic can be simplified to use init for both 0.14 and 0.16, only using the struct literal for 0.15 where the ArrayList was unmanaged.
var new_heap = if (current_zig.minor == 15)
std.ArrayList(Payload){}
else
std.ArrayList(Payload).init(alloc);
| var parse_stack = if (current_zig.minor == 14) | ||
| std.ArrayList(ParseState).init(allocator) | ||
| else | ||
| std.ArrayList(ParseState){}; | ||
| try std.ArrayList(ParseState).initCapacity(allocator, 0); |
There was a problem hiding this comment.
Similar to the previous comment, std.ArrayList(T).init(allocator) should be used for Zig 0.14 and 0.16 to avoid the unnecessary try or catch unreachable on an operation that doesn't need to allocate memory initially.
var parse_stack = if (current_zig.minor == 15)
std.ArrayList(ParseState){}
else
std.ArrayList(ParseState).init(allocator);
| var input = if (builtin.zig_version.minor == 14) | ||
| std.ArrayList(u8).init(allocator) | ||
| else | ||
| std.ArrayList(u8){}; | ||
| std.ArrayList(u8).initCapacity(allocator, 0) catch unreachable; |
There was a problem hiding this comment.
The use of initCapacity(allocator, 0) catch unreachable throughout the test suite is repetitive and less idiomatic than using init(allocator) for versions where the ArrayList is managed (0.14 and 0.16).
var input = if (builtin.zig_version.minor == 15)
std.ArrayList(u8){}
else
std.ArrayList(u8).init(allocator);
- goto-bus-stop/setup-zig 仓库已归档，后续不再维护 - 替换为 mlugg/setup-zig 保证后续兼容和安全 - 避免旧 action 可能引起的集成失效或安全风险
- Add push to main and pull_request triggers for test workflow - Enhance run-name to label pull request runs with PR number - Remove workflow_run and related dependencies to simplify workflow triggers This streamlines CI runs and clarifies execution context for test jobs.
- Replace deprecated `initCapacity(allocator, 0)` with `.empty` for ArrayList compatibility on Zig 0.16+. - Switch benchmarks from `page_allocator` to `smp_allocator` per updated allocator recommendations in newer Zig. - Clarify version differences in documentation related to ArrayList and allocator changes. These updates ensure compatibility across Zig 0.14 to 0.16+.
- Drop scheduled (cron) unit test workflow to reduce unnecessary CI load - Test matrix no longer includes Zig "master" version, only stable releases - Update documentation to match new Zig version coverage
- Add tests for string, bin, ext, and map length limit violations - Add tests for write functions that return short writes to ensure LengthWriting errors are triggered Improve coverage for code paths not tested elsewhere
1 participant
initCapacity()in relevant places to support Zig 0.16, preserving compatibility with older releases.