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

Move Allocator patterns from std.heap to std.alloc #22272

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GrayHatter
Copy link
Contributor

@GrayHatter GrayHatter commented Dec 19, 2024

Because allocations are such a core pattern within Zig, the various Allocators deserve a clear namespace of their own. Additionally a number of the various allocators may be misleading or confusing within the heap name namespace, e.g. FixedBufferAllocator where most of the examples and uses use stack based memory, and not heap.

Additionally moving the various Allocators from heap into std.alloc also improves the stdlib namespace. Where std.heap.ArenaAllocator is now std.alloc.Arena

There's still a large number of places through out that still use std.heap instead of the new namespace. Aliases are provided within std.heap for all of the moved Allocators, as I wanted to solicit some feedback on the PR and approach.

Most uses within alloc have been updated, notably none of the uses in GeneralPurposeAllocator, which uses @import("std") instead of @import("../std.zig"). I didn't want to change this without knowing why.

Also of note on line 1473 of gpa, it appears to test the GPA from standard lib, rather than the current version within the file. I changed this to test the current implementation from the file because there were no comments explaining why, which seemed like a bug.

I assume it's likely I'm missing some other nuance, e.g. I moved and reduced the scope of next_mmap_addr_hint because I couldn't find any other uses.

@mlugg mlugg added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. labels Dec 19, 2024
@nektro
Copy link
Contributor

nektro commented Dec 19, 2024

I like the spirit of this! since this got the proposal label i wanted to add that what if to Avoid Redundant Names in Fully-Qualified Namespaces we skipped adding std.alloc and moved std.heap.ArenaAllocator to std.mem.Allocator.Arena? and/or moved std.mem.Allocator to std.Allocator as well since std.mem is more about slice operations?

@GrayHatter
Copy link
Contributor Author

I started, but then decided not to move std.mem.Allocator into std.alloc I still think it should be moved; and only didn't here because I wanted to try to limit the PR to one controversial change. IMO, std.alloc should exist, and std.alloc should be for gaining access to memory, and std.mem should be for operating on memory you already control. I didn't consider std.Allocator but if I had, I probably would have made that change. It seems to me that Allocator should exist in the top level namespace, the same way ArrayList does.

Because allocations are such a core pattern within Zig, the various
Allocators deserve a clear namespace of their own. Additionally a number
of the various allocators may be misleading or confusing within the heap
name namespace, e.g. `FixedBufferAllocator` where most of the examples
and uses use stack based memory, and not heap.

Additionally moving the various Allocators from `std.heap` into
`std.alloc` also improves the stdlib namespace. Where
`std.heap.ArenaAllocator` is now `std.alloc.Arena`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. 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.

3 participants