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.
Global vars over here looks a bit iffy: shellu/src/shell.zig at 48c495fe37a61dfdf870e274ea1c1681a6c58b30 · nathanielketema/shellu · GitHub
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.
const history_file = env.get("HISTFILE") orelse try Io.Dir.path.join(gpa, &.{
home_path,
".shellu_history",
});
On the orelse branch, we leak the path.
{
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.
std.mem.eql(u8, ctx.command.args[ctx.command.args.len - 1], "&")
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.