Reading a file line by line

[toxic spear]

I was working through some bugs I found in a previous attempt of mine to read files line-by-line. The solution below seems to remedy the problems I was having, but I wanted a second set of eyes on this to see if I've overlooked anything (the buffer size of 1 was simply testing my case where the buffer was too small, I usually have it as 4096):

const std = @import("std");

pub fn main() !void {
    var read_pos: u64 = 0;
    try readFileLines(std.heap.smp_allocator, "foo.txt", &read_pos);
}

pub fn readFileLines(alloc: std.mem.Allocator, file_name: []const u8, read_pos: *u64) !void {
    const fs_file: std.fs.File = try std.fs.cwd().openFile(file_name, .{});
    const stats = try fs_file.stat();
    try fs_file.seekTo(read_pos.*);

    var buffer: [1]u8 = undefined;
    var reader = fs_file.reader(&buffer);

    var writer = std.io.Writer.Allocating.init(alloc);
    defer writer.deinit();

    outer: while (read_pos.* < stats.size) {
        writer.clearRetainingCapacity();

        while (true) {
            const res = reader.interface.takeDelimiterInclusive('\n') catch |err| {
                switch (err) {
                    error.EndOfStream => break :outer,
                    error.StreamTooLong => {
                        const buf = reader.interface.buffer;
                        if (buf.len == 0) return error.StreamTooLong;
                        try writer.writer.writeAll(buf);
                        _ = try reader.interface.discard(.limited(buf.len));
                        read_pos.* += buf.len;
                        continue;
                    },
                    else => return err,
                }
            };

            try writer.writer.writeAll(res);
            read_pos.* += res.len;
            break;
        }

        const line = try writer.toOwnedSlice();
        if (line.len == 0) break;

        std.log.err("line: {s}", .{line});
    }
}

My use case has this function called at different intervals with a read position, so that file reading can be continued from where it last left off

[quaint talon]

why not just persist the file reader, which already tracks and memoizes the read position

[toxic spear]

I suppose I could do that, it doesn't really help reduce the complexity though right?

[quaint talon]

well, you no longer have to manually keep track of a read_pos

also, if you're going to be allocating the lines anyway, you may as well just directly use streamDelimiter

[toxic spear]

does that have the same issue as takeDelimiterInclusive where the reader's buffer can be too small to reach a delimiter?

[quaint talon]

no, it streams the contents that are being read directly into a writer

ie, here it would be something like try reader.interface.streamDelimiterLimit(&writer.writer, '\n', limit)

which, btw, I would probably choose better names lol

writer.writer -> line_buffer.writer

though to be clear this is an extremely inefficient way of doing this

If I wanted to read a file line-by-line, with arbitrarily long lines, I would probably just do this:

fn doStuff(gpa: std.mem.Allocator, r: *std.Io.Reader, line_limit: std.Io.Limit) !void {
    var line_buf: std.Io.Writer.Allocating = .init(gpa);
    defer line_buf.deinit();

    while (true) {
        line_buf.clearRetainingCapacity();
        _ = try r.streamDelimiterLimit(&line_buf.writer, '\n', line_limit);
        const next_byte = r.peekByte() catch |err| switch (err) {
            error.EndOfStream => break,
            else => |e| return e,
        };
        std.debug.assert(next_byte == '\n');
        r.toss(1);

        const line = line_buf.written();
        std.log.err("line: {s}", .{line});
    }
}

usage being

const file: std.fs.File = try std.fs.cwd().openFile(file_name, .{});
defer file.close();

var fr_buf: [4096]u8 = undefined;
var fr = file.reader(&fr_buf);
try doStuff(gpa, &fr.interface, .limit(whatever));

[toxic spear]

hm I could probably turn this into an iterator too which would be nice to reuse

[quaint talon]

the point of the reader and writer interface is that they are your "stream primitives"

I would perhaps mull over that immediate desire to abstract

it's not always a positive pursuit

[toxic spear]

Well I need to do this same thing in a few places in my code, just parsing these lines and sending them off to clients. Would be nice to not duplicate the code again

Any problem with using .unlimited here?

[quaint talon]

mostly just a question of sanity

it generally isn't a good idea to allow potentially unbounded memory allocation

[quaint talon]

I would just say to this: maybe just write the code first, and see how it is. premature abstraction is amongst the roots of all evils. in committing it, you are trying to predict what you are going to need, which humans are notoriously not very good at doing. you'll generally find more success by identifying patterns after the fact, sussing out the abstraction which perfectly fits the code, rather than trying to fit the code to an abstraction

[toxic spear]

I think it's very reasonable here
With your code being this simple to read line by line, I'm surprised to not find something like this around as an example. The new reader api has been very confusing to do even basic tasks

[quaint talon]

my code is simple because I didn't try to immediately abstract away all of the details I don't immediately understand

read the doc comments, read the code, try to understand the tools

and try to actually use them, rather than trying to shoehorn them into something that fits your preconceptions of what you're used to

[toxic spear]

I have, it's just more difficult to navigate

[quaint talon]

you can argue it's reasonable, but I will again re-iterate: no matter how good you think you are at predicting what you'll need, as soon as you go beyond certain levels of complexity, your prediction is as good as a coinflip

[toxic spear]

I'm very sure of my use case needing to read line by line

I know a lot of people have been having issues grasping this new reader/writer api, too

[quaint talon]

sure, not arguing about the details of functionality, I'm referring to your instinct to "put this into an iterator". abstracting away details doesn't yield simplicity, as your original code clearly demonstrates

[toxic spear]

My original code wasn't an abstraction at all, I was just trying to wrestle with the api to read a line from a file

[quaint talon]

it very much is an abstraction; it attempts to hide away all the details of using the reader/writer API and filesystem calls, exposing a superficially simpler API

[toxic spear]

That wasn't the intent at all, it was just a code sample for a help thread. There's no extra abstraction inside of readFileLines

[quaint talon]

I'm saying readFileLines is an abstraction

and it is the wrong abstraction

[toxic spear]

It's not an actual real function I'm using in my program, it's just example code for this help thread. I suppose I could have just put it all in main just the same, there's no intent to abstract

I just wanted to figure out how I can read lines from a file

[quaint talon]

fair enough, but I stand by my words: don't rush to abstract this all away by wrapping it into an iterator

just write the code and see if there's a good pattern to give a name to afterwards