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

Remove allocators from std.fs.{Dir.open, deleteTree} #3249

Closed
wants to merge 1 commit into from
Closed

Remove allocators from std.fs.{Dir.open, deleteTree} #3249

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 19, 2019

While I believe the tests pass fine for FreeBSD on my machine (because the ominous event.channel.test hangs forever on FreeBSD), I need to fix the Linux and Darwin tests and adapt my changes to them to see if they pass as well.

Hence this is still Work-In-Progress. But I'm pretty excited because I finally got somewhere and I want to share my work before merging. Also any advice on cleanup/refactoring/etc is appreciated.

Since this changes some of the API in std, it's a breaking change! 💣 🛠️

Closes #2885 and closes #2886

@andrewrk
Copy link
Member

because the ominous event.channel.test hangs forever on FreeBSD

Can you make sure there is an issue open for that, and then open a pull request to disable this test only on FreeBSD, with a comment citing the issue?

@andrewrk andrewrk added this to the 0.6.0 milestone Sep 19, 2019
@daurnimator daurnimator added the standard library This issue involves writing Zig code for the standard library. label Sep 21, 2019
@ghost ghost changed the title [WIP] Remove allocators from std.fs.{Dir.open, deleteTree} Remove allocators from std.fs.{Dir.open, deleteTree} Sep 22, 2019
@ghost
Copy link
Author

ghost commented Sep 22, 2019

I have a thought. To both solve the problem of removing the allocator from both function calls and allow the path size to grow (in case of mount-on-mount paths and EINVAL), could I just make a "global FixedBufferAllocator" on the top of std/fs.zig and use that? I feel that would be the simplest solution.

@andrewrk
Copy link
Member

I have a thought. To both solve the problem of removing the allocator from both function calls and allow the path size to grow (in case of mount-on-mount paths and EINVAL),

EINVAL only occurs when the given buffer is not large enough. If we give a large enough static buffer we can assert that EINVAL is unreachable.

What's this mount-on-mount issue?

could I just make a "global FixedBufferAllocator" on the top of std/fs.zig and use that? I feel that would be the simplest solution.

That would be a classic blunder. Many libc APIs have gone this route and regretted it. Using global memory in this way makes the function non-thread-safe.

@ghost
Copy link
Author

ghost commented Sep 23, 2019

I see. I won't do that then. I think I'll just give the buffers 8192 bytes and call this PR a day because I feel like I've been focusing on this far too long. I would like to work on another standard library issue.

The mount-on-mount issue is a concern for the possibility of an extended path sizes from mounting a medium (in *nix) and reaching to the far end of the place-of-mount size + full path size of the medium. However I think this is a minor concern. If an issue like this pops up in the future for some use case, I think simply doubling the buffer size will suffice.

@ghost
Copy link
Author

ghost commented Sep 23, 2019

I forgot the unreachable EINVAL, that will be coming up soon.

@andrewrk andrewrk modified the milestones: 0.6.0, 0.5.0 Sep 27, 2019
@ghost
Copy link
Author

ghost commented Sep 28, 2019

@andrewrk Friendly reminder to merge before Monday. 😟

@andrewrk
Copy link
Member

I did a first pass at reviewing this pull request. Nice work, @stratact.

There are some adjustments I'd like to make and I'd like to play around with it a little bit before merging it. I think it will be better to do this at the beginning of the 0.6.0 release cycle rather than making a big, risky (and exciting!) change to a core std lib API days before releasing 0.5.0.

I know you wanted to get this into the release, but I think it will be better for stability purposes to do it in 0.6.0.

@andrewrk andrewrk modified the milestones: 0.5.0, 0.6.0 Sep 28, 2019
@ghost
Copy link
Author

ghost commented Sep 29, 2019

Fair enough, that's very reasonable. Thank you for understanding my feelings as well.

@andrewrk andrewrk removed this from the 0.6.0 milestone Oct 1, 2019
@andrewrk andrewrk closed this in e839250 Oct 22, 2019
@ghost ghost deleted the no-dir-allocators branch October 22, 2019 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
2 participants