Skip to content
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

std/ArrayList: Allow ArrayList(u0) to be created #8632

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

zbanks
Copy link
Contributor

@zbanks zbanks commented Apr 28, 2021

I encountered this issue when writing a type-generic datastructure: if my datastructure is generic over type T and internally has an std.ArrayList(T), then it fails to compile if T is zero-sized. It fails to compile because the allocator cannot allocate a slice for zero-sized types (e.g. "error: no align available for type 'void'").

Example code, godbolt link with errors

test {
    const std = @import("std");

    var voidList = std.ArrayList(void).init(std.testing.allocator);
    defer voidList.deinit();

    try voidList.append(void {});
    for (voidList.items) |_, index| {
        // Do something with `index`
    }
}

Although ArrayList(void) sounds dubious, I think a motivating example is creating a HashSet(Key) from a HashMap(Key, void), which is something the docs specifically mention. The current std implementations store the Key & Value pairs next to each other, in AoS form, which sidesteps this issue. However the problem would be encountered when implementing #8363, where the pairs are stored in separate arrays in SoA form.

I also wanted to mention that issue #7221 may impact this? I'm not fully sure of the ramifications of making zero-sized types have alignment 1. (Would it just fix this problem at a lower level? Would it break my patch's tests?)


For my implementation, I have ArrayList(void) still track the length of the list, but it never allocates additional memory. The list.items slice ptr is never updated, but the len is, so it can still be iterated over with for, etc. It sets list.capacity to maxInt(usize) (and never updates it).

I also wanted feedback on my code specifically:

  • Is ArrayList the right place to fix this, or should the fix be at the Allocator level? (That sounds "scarier," more potential for a breaking change?)
  • Is @sizeOf(T) == 0 the best way to check for zero-sized types? I saw in the docs that comptime-only types, like type and comptime_int, would also match this test -- but it doesn't make sense to use an ArrayList for them.
  • I set capacity to maxInt(usize) and never update it, even when calling shrinkAndFree(...) -- I think this makes sense? But it does violate an implied invariant that list.shrinkAndFree(n); list.capacity == n
  • Is it more clear to have the test use void instead of u0? (Or are there other cases I should cover?)

@andrewrk
Copy link
Member

I'm definitely interested in solving this use case, however before merging this PR I want to look into solving it at a deeper level (perhaps in the allocator) rather than putting special case code into ArrayList to work around the issue.

@SpexGuy
Copy link
Contributor

SpexGuy commented Apr 28, 2021

I made #6881 a while back to fix a bunch of these issues in the allocator, but ultimately I closed it because it's mostly workarounds for #6706 and related issues like #6983, #6947, #6937, #6936, and #6861.

@marler8997
Copy link
Contributor

Just FYI, when I implemented the allocator refactor, I recall some code assumed and depended on the allocations sizes passed to allocFn and resizeFn to be non-zero size. I don't know if those assumptions are still baked into the interface/implementation but just know that at some point in time they were.

@Vexu Vexu added the standard library This issue involves writing Zig code for the standard library. label Jun 13, 2021
Enable creating ArrayList with zero-sized types.
This type still tracks length, but does not allocate
additional memory.
@andrewrk andrewrk merged commit 9c028b2 into ziglang:master Jul 28, 2021
@andrewrk
Copy link
Member

Thank you @zbanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants