Skip to content

Update for breaking Zig Allocator API change #2179

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from

Conversation

mochalins
Copy link
Contributor

@mochalins mochalins commented Feb 9, 2025

Updates ZLS to compile and pass tests with the ziglang/zig#20511 PR merged in Zig master.

Of note is the new remap function in the binned allocator; it simply calls resize internally à la remap functions introduced for several allocators in the Zig standard library (see e.g. FixedBufferAllocator), in the aforementioned PR.

This can also be replaced with a naive implementation that copies resize, but internally calling the underlying allocator's remap. I can make this change upon request, or this can be left for a followup PR (potentially leaving room for an entirely different implementation to make use of remap's looser guarantees).

Also includes the minor popOrNull -> pop change in ziglang/zig#22721

@mochalins mochalins force-pushed the zig_master_update branch 2 times, most recently from c7b626b to b1f3bc4 Compare February 9, 2025 08:09
@bullekeup
Copy link

Hi

I am new to zig and zls and was about to open a PR with the same changes (my first try to contribute in the zig ecosystem), but @mochalins has been quicker ! My changes just have the difference highlighted in my previous comments.

I don't want to open a conflicting PR so just take my suggestions if you want to

@mochalins
Copy link
Contributor Author

Thank you for the suggestions @bullekeup ! I hadn't noticed that helper function for alignment. I'd love to incorporate your suggestion, but it'd be even better if you would show up in the commit history to get proper credit; would you mind opening a PR against my fork branch so that I can accept it and have your commit show up here?

@mochalins
Copy link
Contributor Author

Requires one more update to handle latest changes from master; will fix shortly.

The `popOrNull`->`pop` changes affect `master` build runner. Minimum
runtime Zig version must be updated accordingly.
@mochalins
Copy link
Contributor Author

@Techatrix

The latest changes to popOrNull -> pop on Zig master affected the master build runner. As such, the mach-latest build runner checks on CI were failing; to address this, I created a separate mach-latest.zig file in build_runner that runs on the CI, and I did not add another master build runner check to the CI matrix due to the comment that master build runner is checked in tests.

I wasn't able to keep the build runner to one master.zig file, as comptime branching on the builtin Zig version did not properly compare the pre part of the version.

Are these changes acceptable?

@Techatrix
Copy link
Member

A new allocator (std.heap.SmpAllocator) has been recently add to the standard library. I would rather have ZLS use it instead of the binned allocator.

I wasn't able to keep the build runner to one master.zig file, as comptime branching on the builtin Zig version did not properly compare the pre part of the version.

ZLS has been comparing zig versions at comptime for a while now without issues. I tested the following change which worked for me:

-        while (stack.popOrNull()) |step| {
+        while (switch (comptime builtin.zig_version.order(std.SemanticVersion.parse("0.14.0-dev.3181+914248237") catch unreachable)) {
+            .lt => stack.popOrNull(),
+            .eq, .gt => stack.pop(),
+        }) |step| {

This could be happening because the minimum_runtime_zig_version has been updated which means that ZLS won't run the build runner on older zig versions. The mach-latest.zig file is only used in the CI. It is never used in ZLS itself making it useless.

To not delay these changes any further, I will apply them myself but I still appreciate all the effort you have put into this PR.

@Techatrix Techatrix closed this Feb 11, 2025
@mochalins mochalins deleted the zig_master_update branch April 30, 2025 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants