Skip to content
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

Merge planning #3

Open
iddev5 opened this issue Sep 26, 2021 · 7 comments
Open

Merge planning #3

iddev5 opened this issue Sep 26, 2021 · 7 comments

Comments

@iddev5
Copy link
Collaborator

iddev5 commented Sep 26, 2021

In order to merge zig-ar with this, some minor changes/addition needs to be done.

  1. Add header and contents field to ParsedFile
pub const ParsedFile = struct {
    name: []const u8,
    header: Header,
    contents: []u8,
};
  1. Contents need to be stored
pub fn parse(self: *Archive, allocator: *Allocator, stderr: anytype) !void {
    ...
    // try reader.context.seekBy(try fmt.parseInt(u32, mem.trim(u8, &archive_header.ar_size, " "), 10));
    var contents = try self.allocator.alloc(u8, size);
    try reader.readNoEof(obj_file.contents);
    ...
}
  1. Merging these things (related to 1 actually), though it is not that mandatory
archive_headers: std.ArrayListUnmanaged(format.ar_hdr),
parsed_files: std.ArrayListUnmanaged(format.ArchivedFile),
  1. Minor nitpick
pub const ar_hdr = extern struct {
    ar_name: [16]u8,
    ar_date: [12]u8,
    ...
};

We can just have normal names in Zig convention here. The reason is that freebsd manpage lists this struct in terms of its own symbols, its not any kind of contention afaik. I decided to have mine as follows:

pub const Header = extern struct {
    name: [16]u8,
    mtime: [12]u8,
    ...
}

Let me know if something seems odd, I will start working on merging tomorrow

@iddev5
Copy link
Collaborator Author

iddev5 commented Sep 26, 2021

On some side notes, zar is currently missing any hints to thin archives.
Also, this superceeds 2

@moosichu
Copy link
Owner

Cheers! All of these changes look sensible to me. I'm happy to go with zig-naming, I was only copying the convention for that struct in the compiler, but you are right that it differs based on the implementation.

I will make the changes as requested on my end tomorrow morning to help prepare for the merge if that works. :)

@moosichu
Copy link
Owner

Actual code merge is done 🎆 #4 !

I think before this is considered wholly complete - the results of the merge need to be cleaned-up so that everything works using similar conventions.

This is both in style (using same names for variables etc.) and functionality (using file-based as opposed to in-memory representation for files). I'm happy to help with either of these. But during the week my time availability is going to be limited (I will be able to work on this for a couple of hours some mornings).

But having the actual code merged-in is a great first step! :D

@iddev5
Copy link
Collaborator Author

iddev5 commented Sep 27, 2021

I agree with the content representation part. I will continue to work on this project as a whole and try to resolve as much as I can.

@iddev5
Copy link
Collaborator Author

iddev5 commented Sep 27, 2021

One more thing is that I suggest marking the struct Archive managed. This is because

  1. Explicitly passing the allocator in all function is tedious
  2. We currently aren't dellocating any memory, which is bad

@moosichu
Copy link
Owner

OK - so I'm not sure how sensible this is, but my current plan was to structure the program around the fact that ar is a simple program. As long as we aren't allocating more memory than is needed persistently anyway, a single arena allocator that is simply cleared when the program terminates should be fine I think - which means that explicit deallocations simply aren't needed.

I will double check with @kubkon to draw on their experience as well just in case.

@iddev5
Copy link
Collaborator Author

iddev5 commented Sep 27, 2021

Well not only that but we aren't closing file handles either right now...

On the actual topic, we are passing the arena allocator explicitly to the different functions, which means that someone could technically pass a different allocator to it and it would leak(?). I know this situation is unlikely to actually ever occur as the whole of zar is planned to be inside zig one day, but why even keep loose knots :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants