Idiomatic code?

I’m interested in what the idiomatic way is to deal with deinit() requiring a non-const in cases like the code below. The code below won’t work, because the deinit function is deinit(self: *Action, gpa: std.mem.Allocator)

while (action_queue.popFront()) |action| {
    action.perform();
    action.deinit(gpa);
}

I can’t do |*action| in this case. My solution right now is to copy action to a var, as below:

while (action_queue.popFront()) |action| {
    action.perform();
    var a = action;
    a.deinit(gpa);
}

Is there anything better in terms of idiomatic zig? Ignoring the copy (which I hope gets optimized away, though I haven’t checked), I don’t even like having to come up with a unique variable name here just for this purpose.

Thanks!

I guess you could change your queue to have methods front (where front would return a pointer to the first entry) and removeFront, then you could use this:

while (action_queue.front()) |action| {
    action.perform();
    action.deinit(gpa);
    action_queue.removeFront();
}

Otherwise if you want to keep using popFront I am not aware of a different solution than what you already found.

1 Like

Interesting - your version at least means I’m not worried about the copy or the additional variable that needs naming. It’s maybe ugly in a different way though since the logic of draining the queue is less obvious - without the removeFront() call it’d be an infinite loop.

1 Like

Another solution would be iterating over the backing array of the queue, calling perform() and deinit() on each and then resetting the queue to zero size at the end. This is a double ended queue, and using the internal backing array directly for this feels wrong. The iterator I have on the queue doesn’t help, since that returns elements, not pointers. I’d need an additional iterator that returns pointers. Maybe that’s least ugly of the options?

1 Like

I know it is worthy of criticism, and it does feel dirty to do, but for deinit functions, I will actually use @constCast if it is necessary. The rationale I use is that the object is being destroyed, and it shouldn’t be considered valid after the call anyways.

This is of course very situational. The only time that I do this is if the type is otherwise capable of being const qualified for all of its other functionality.

2 Likes

Maybe another approach would be to use a custom crafted iterator:

var iter = action_queue.consumeIterator(gpa);
while(iter.next()) |action| action.perform();

The iterator would be written in such a way that it returns the pointer to the first element and starting from the second iteration it would call action.deinit(gpa); action_queue.removeFront(); before getting the pointer. That way the pointer would be valid for the whole body of the while loop. The last iteration that returns null would clean up the last action item.

I guess here the downside is having to create this custom iterator, but other than that I kind of like it.

2 Likes

@constCast does neatly solve the problem, as dirty as it is. I’ve never used it before, and am now questioning why it’s called @constCast since it actually does the opposite. :grinning:

I think it still is worth to try out that solution, maybe accessing the internal array should be seen as a valid use case, similar to how .items is part of the public interface for std.ArrayList, Zig doesn’t have private fields for a reason, so maybe this shouldn’t feel wrong at all.

Looping over the internal array processing all elements and then setting the size to zero, seems like the simplest approach that would likely have the best performance, because it doesn’t use single element functions to process it piece by piece. From a data-oriented programming stand point, iterating over the array seems like the best thing to do.

2 Likes

I totally agree in general. In this case it’s a double ended queue, so isn’t actually guaranteed to have a single contiguous backing slice. That means you need to know a lot more about the internals of the queue than seems reasonable. Still doable, but uglier in this particular case than I’d like.

What about adding a bulk processing api to the double ended queue, that gives you either an iterator (if the length isn’t known before iterating) or a slice over all the internal arrays?

In the most general case it could be something like this:

var iter = action_queue.consumeAll();
while(iter.next()) |chunk| {
    for (chunk) |*action| {
        action.perform()
        action.deinit(gpa);
    }
}

The iterator could than either remove whole chunk-arrays once they were processed (before the next iteration starts) or clear all arrays at the end, depending on what turns out better.

2 Likes

That sort of begs the question, should deinit() accepts a regular pointer or a const pointer? Is freeing memory is an act of modification? When Thanos snapped his fingers, did Peter Parker become not-Spiderman? No, he just ceased to exist altogether. Being denied existence is not the same as being altered, I would argue.

1 Like

It’s typically done to allow poisoning the memory in an effort to make use-after-free easier to detect. One of many examples in the standard library:

Can’t really speak to how effective it has been in practice. It’ll likely become much more relevant after debug safety feature: runtime undefined value detection · Issue #211 · ziglang/zig · GitHub is implemented

3 Likes