I’m showcasing shellu, a POSIX compatible shell that I’ve been working on to improve my zig skills. I’ve worked on smaller zig projects before, but this is my first attempt at a mid sized project.
I’d really love to get feedback on my coding skills. It’s become pretty hard to get useful code reviews these days, and I’m a bit skeptical of AI feedback because I can’t fully verify it. I don’t have a good way to judge the experience or reputation behind the advice, so feedback from experienced zig developers would mean a lot.
Feel free to be as nitpicky as you want, I’m here to learn.
Not really a review, just some things I’ve noticed while skimming the code:
var shell: Shell = try .init(init.io, init.gpa, init.environ_map);
I personally have a fairly strong preference to write this as = try Shell.init. I often want to grep for all of the places where a thing is initialized, and, if you use .init everywhere, you can’t grep trivially. In other words, I tend to use .name syntax only when the name is sufficiently unique.
I understand that this is due to using readline, but I’d personally be motivated to find a C-less solution here. I also don’t like how redline doesn’t use arena in the main loop.
{
var key_it = shell.variables.keyIterator();
while (key_it.next()) |key| shell.gpa.free(key.*);
var val_it = shell.variables.valueIterator();
while (val_it.next()) |val| shell.gpa.free(val.*);
shell.variables.deinit();
}
This kind of loop in deinit always smells to me — why are we managing individual memories one at a time? Can all keys&values share the same storage perhaps? This is the kind of code that RAII writes for you, and this is precisely why RAII tends to have negative effect on the program design with respect to memory.
This I think is the only usage of arena inside Shell, and we never reset this arena, so, the pipes array list effectively leaks.
this is ctx.command.args[ctx.command.args.len - 1] == '&'
Do we close this file? Do we close this file if we get an error down the line?
In general, my main take away is that I don’t quite understand the resource management strategy inside Shell.zig, I see quite a few of individual allocations/deallocations, and such code tends to get rather tricky to get right in Zig.
About the initialization. I am often in doubt what to do! When ‘grepping’ .init() in my code I get too much.
Mostly i dislike it when I read type infered variables / consts:
const thing = initializeSomeThing(); // what is this thing?
So I prefer heavily:
const thing: Thing = initializeSomeThing(); // ok now we know: it's a Thing.
For structs there are basically 3 options:
var shell: Shell = try .init(); // not 'greppable'.
var shell = try Shell.init(); // ok in this case, we know what it is.
var shell: Shell = try Shell.init(); // maybe too much.
In longer statements the .init() can become kinda unreadable as well:
const a: Thing = switch (...) {
a => .init(7),
b => .init(8)
}
Curious how other people think about it. But maybe that does not belong in this thread.
Thank you @matklad for taking the time. I’m a big fan of your blog posts and your Ironbeetle show. I have to say, I was genuinely surprised and excited when I saw that you replied to my post
Yeah, I think “greppability” should be one metric for evaluating code quality. I’ll keep that in mind.
Global variables, like you mentioned, came from not finding a better way to interact with the GNU readline library, especially when I needed to pass shell state to the library. I could’ve gone with a different native zig completion library, but I wanted to learn more about zig’s interoperability with C. In fact, this led me down the rabbit hole of building C libraries in zig, and I ended up writing Readline packaged for zig.
Regarding the orelse branch, I need to be extra cautious when it comes to dealing with borrowed memory and owned memory.
I agree, I should be using shell.arena more to get the benefits of bulk allocation/deallocation. It’s very tricky indeed to get memory lifetimes right.
Question: how do you guys deal with resource management in zig? Any patterns you use to avoid these kinds of memory bugs?
I don’t think he was necessarily referring to arena allocation. If you haven’t seen it Andrew had an awesome talk on this topic: Video: Programming without pointers
I have also written some of the things matklad mentioned hehe.
I’m skeptical about the value of ‘grep searchability’. There can already be a lot of types with the same name in different namespaces, and Zig allows a type to be renamed when imported into another file. Relying on grep to figure out where a type might be initialized could itself be a bad practice, and we shouldn’t encourage that by writing code that’s just easier to grep.
