commanderx16 / x16-emulator

Emulator for the Commander X16 8-bit computer
384 stars 61 forks source link

Directory recogition and traversal support for hostfs #466

Closed mooinglemur closed 1 year ago

mooinglemur commented 1 year ago

This PR requires review before accepting. I have no way to test whether things work as expected in OS X, but I was able to test Windows and Linux.

My rusty C is likely not optimal, and I might be using weird code patterns that have better library functions, but the changes seem to work and I think I was careful enough that leaks do not accumulate. But there is a bit of new code that touches the filesystem that might have portability concerns.

Changes include:

irmen commented 1 year ago

Just piping in to say I've been testing this with several of my programs (image viewer, a game, file based assembler, and the x16 shell) and many issues they had on host filesystem have been resolved. Most if not all of the functions now work on host fs where they only worked on sdcard image before.

irmen commented 1 year ago

Maybe it also should handle the "@:" filename prefix now, i.e. overwrite existing file Instead it still treats it as regular part of the filename.

mooinglemur commented 1 year ago

There is no @: or wildcard handling yet. I had originally planned that to be in a future PR, but I can go ahead and work on that support.

irmen commented 1 year ago

Regarding the "@:" and pattern matching in particular: I'm fine either way, this PR is large enough as it is.

mooinglemur commented 1 year ago

I feel that the state of this PR is better than what exists in master, and doesn't seem to break anything that already exists, but it does of course need more review and testing to make sure the new features work as expected

indigodarkwolf commented 1 year ago

Maybe this speaks to my (potential lack of) attention to style, but stylistically nothing stands out to me as needing to change.

The use of dynamically allocated strings strikes me, personally, as an unusual thing, but what with the POSIX functions that can't make path length guarantees anyways and have to be called in such a fashion that they return dynamically allocated strings that the caller is responsible for free()'ing, well... I feel all I can do is check that everything appears to be getting cleaned up appropriately, and I didn't spot any memory leaks.

indigodarkwolf commented 1 year ago

Awesome. So many thanks for all the work here!

irmen commented 1 year ago

Hurrah !

indigodarkwolf commented 1 year ago

There is a coda, which I'll leave here though it should probably be elevated to an issue (maybe a good first-timer issue, even?). To remove some, but not all, of the dynamic allocations of strings, we should consider replacing at least some of the allocs with alloca. alloca performs a dynamic allocation onto the function stack, meaning you don't need to free it later to clean it up. Very useful as long as we're not talking about many kilobytes or larger.

irmen commented 1 year ago

is alloca still a thing in C++ btw?

indigodarkwolf commented 1 year ago

Sure is.