-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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: rework uefi allocators #22818
base: master
Are you sure you want to change the base?
std: rework uefi allocators #22818
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this is the right size for a PR like this :)
Two high level concerns:
- All allocators, including platform-specific ones like
WasmAllocator
currently live instd.heap
. If we're moving these around that should be where. - Why are we adding additional abstractions on top of the UEFI-provided allocator functions? In general that's a good thing if it improves ergonomics of those APIs from a Zig perspective, but in this case no one but the allocator implementation should touch those, and I feel in that case the extra layer is not justified.
Will do
I'm not 100% sure, this originally came from @truemedian. |
e0ab1e2
to
da5fad6
Compare
Which additional abstractions are you referring to?
|
7efc07c
to
a9862db
Compare
The ones in |
Ah, thats because this came out of a branch where the goal was to make |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase this after #22820 and provide a compelling use case for the custom layer in boot_services.zig
or remove it.
/// Allocates memory in pages. | ||
/// | ||
/// This allocator is backed by `allocatePages` and is therefore only suitable for usage when Boot Services are available. | ||
pub const PageAllocator = struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The namespacing here is awkward, this ought to be a file-as-struct (UefiPageAllocator.zig
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why file as a struct when there's multiple allocators?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because std.heap.UefiPageAllocator
is much nicer than std.heap.uefi_allocators.PageAllocator
(which violates https://ziglang.org/documentation/master/#Avoid-Redundant-Names-in-Fully-Qualified-Namespaces). Why do you want both allocators in the same file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because
std.heap.UefiPageAllocator
is much nicer thanstd.heap.uefi_allocators.PageAllocator
Wouldn't std.heap.uefi.Page/Pool
be nicer?
Why do you want both allocators in the same file?
Just so there isn't 3 more files that could fit into 1.
Already been rebased.
I don't have one unless @truemedian does. I think it just makes the API easier to use and it's already there. Shouldn't need additional maintenance since UEFI's API doesn't really change in that section. I probably will just remove it. |
I see at least one reference to |
a9862db
to
33af113
Compare
Fixed, strange that one didn't get caught when I tried compiling |
For the most part Zig follows YAGNI principles. |
Split off from #22226 as suggested by @linusg. This reworks the allocators to be more friendly.