-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Convention, deinit doesn't invalidate #6322
Comments
It's worth noting that |
@Tetralux I like the term |
should a const reference to a resource be able to release it? |
This proposal suggests that all |
This follows the convention described here: ziglang/zig#6322.
I'm happy with |
There are already types that have a way to free resources without invalidation. For example, |
HashMap has pub fn deinit(self: *Self, allocator: *Allocator) void {
self.deallocate(allocator);
self.* = undefined;
} so it is doing the convention that deinit invalidates. I'm confused what you're trying to communicate. |
The proposal is to establish a convention that allows you to "cleanup type resources" without invalidating them. |
HashMap does not have a deallocate function. It is private. Also the deallocate function does invalidate the memory, it just does not additionally have the safety of setting to undefined. If you tried to use the memory after deallocate, it would be just as broken as if the memory had been set to undefined, just slightly harder to debug. |
Oh shit I didn't realize that. Well then at least you're consistent, wrong in my opinion, but consistent :) |
From an api perspective, a freeing function must do one of two things:
From a safety perspective, a freeing function of the first form should always leave pointers as const MyType = struct {
x: std.ArrayList(u32),
y: std.AutoHashMap(u32, u32),
z: u32,
pub fn deinit(self: *@This()) void {
self.x.deinit();
self.y.deinit();
self.z = undefined;
}
}; Additionally, writing undefined communicates additional information that enables optimizations. So removing the assignment may be a pessimization. The only remaining potential downside of this api is that it requires a pointer to mutable memory. However, this is not really a problem in practice.
In the specific cases we're looking at, like ArrayList and HashMap, an immutable one is useless because you can't modify the |
Thanks for the very thorough analysis/explanation here @SpexGuy. I concede. |
There's one more thing I thought of. As an example, I use "const file" alot. Currently |
This depends a lot on what That said, I think we can take this on a case by case basis. If To be clear, the decision here isn't "everything should invalidate", it's just "invalidation should be preferred, but decided on a case by case basis". |
The problem I've found is that non-constness proliferates. Off the cuff idea: what if assigning |
Yeah, this phenomenon is sometimes uncharitably called "const poisoning", and it's one reason that extending #224 to pointers is a really bad idea. However, unlike languages like C++ or D where But also, the only real objection I'm seeing here is about backwards compatibility, which is not really a reason yet for std lib code. None of it is stable. |
This is a variation of #6299 that @andrewrk suggested be a separate proposal.
Proposal
This issue proposes that by convention,
deinit
never invalidates memory and always takesself
as a value. So instead of this:we use this:
Furthermore, in the cases where invalidation is more than just
self.* = undefined
, it's proposed that a type can define a function namedinvalidate
to contain type-specific invalidation logic, i.e.Why?
The reasoning for this convention appeals to the Single Responsibility Principle. Currently in the standard library there is a convention that
deinit
perform 2 operations:However, there are use cases where code may want to release resources, but not invalidate memory. Since
deinit
always does both, programs may not be able to use the struct in the way that makes sense in their situation. For example, this convention prevents programs callingdeinit
onconst
instances. Say we have aPage
type that wraps a page address:Because
deinit
invalidates memory, it must take a mutable reference. This prevents a program from being able to calldeinit
on a const version ofPage
:For nested types, this can also cause the same memory to be invalidated multiple times when a type calls
deinit
on its members but also invalidate its own memory withself.* = undefined
.Foo
containsBar
, which containsBaz
, which containsBuz
, all of which calldeinit
on all their members and invalidate themselves, this would meanFoo
would invalidateBuz
4 times.You will also find that throughout the standard library, some types choose to invalidate memory inside
deinit
while other do not. These variations are likely caused by differing programmer sensibilities and different cost/benefit analyses for each case. The issue here is that the type definition itself doesn't have the full picture, only the user will. By establishing that types never force invalidation indeinit
, this puts the "responsibility" for deciding whether to invalidate on the party with the all the information, the user. This is the same reasoning that makes Zig's allocators work so well, it puts the responsibility of deciding an allocation strategy on the user, not the functions that only need to allocate memory.Note that some types in the standard library have already addressed these issues. For example
HashMap
defines thedeallocate
function,StreamServer
defines theclose
function, which both release resources without invalidating. This proposal aims to help developers do the "right" thing by default, by allowing the user to make this decision insteading of forcing one on them. By establishing thatdeinit
should always takeself
as a value type, it keeps invalidation and resource deallocation separate.What about Invalidation?
In the common case that a user wants to release resources and invalidate memory, a common function can be implemented to do both, i.e.
Usage:
Some may not like that this function is no longer a member function. If this is the case, we could also define it like this:
Usage:
The text was updated successfully, but these errors were encountered: