Add a std.mem.Allocator argument to the type definition of every custom function parameter?
p7r0x7 opened this issue · 8 comments
It doesn't have to be used, but it should be available if necessary.
I like this idea, but we'll need to game plan it's implementation. The biggest sticking point is determining how the allocator would be provided when the function is actually called. This is due to the parse_fn being a callback, which is called by the library itself and not the library user.
If you can come up with a small example of how you'd use this, that would make it easier to implement I think.
Well, what I was thinking was that because a) libraries shouldn't supply allocators and b) an allocator is already required to be passed to the library, you could just c) leverage the allocator the library already has been permitted to use and just make it something passed to the parse_fn, usage_fn, and help_fn from within cova, whether or not the end user actually needs to use it in their parse_fn, usage_fn, and help_fn's; if they don't need it, it's just a pointer that gets passed around and doesn't slow anything down, and if they do, they can use it just as you, the developer of the library would.
I want to be able to build strings at runtime from the runtime arguments as part of the parsing of values, and potentially in the other functions, but I've been taking everything one step at a time for the most part, as you've undoubtedly observed.
I am new to Zig and using allocators, and I didn't want to break the very intelligent convention of allowing the end user to decide how to allocate objects, so that's why I thought that for any function the end user can supply to the library, an allocator should be made available.
Using the provided allocator is the best way I can think of too. I'm fully on board with that. The only additional change will be adding an _alloc field to Options and Values that's a pointer to their Command's _alloc.
I'm also fairly new to memory allocation (as well as Zig) and agree that Zig's allocator paradigm makes a lot of sense. I think implementing this proposal by using the Allocator provided for initialization also falls right in line with how many items in the standard library work (like std.ArrayList).
I'll push forward with this idea and let you know when I have it implemented!
I haven't read much of the stdlib yet but _alloc as an internal variable your end users can generally access is kinda weird, so I wonder if there's a better way; specifically for parse_fn I was thinking you could just change the type definition from ?*const fn([]const u8) anyerror!ChildT to ?*const fn(mem.Allocator, []const u8) anyerror!ChildT and adjust the code within cova accordingly.
Except for within these functions and via the object that the user initially passed, cova's allocator shouldn't really be accessible by the user.
So, I'm generally inclined to agree, however Zig does not, and will not, have private fields. As such, there isn't a way to reasonably hide the allocator that's not overbearing. Idiomatic Zig dictates that fields should be openly available to get/set and that their usage should be defined by good naming and documentation. As such, I use the _ prefix and bottom line notes in documentation comments to make it as clear as possible that certain fields are either read-only or wholly internal.
You can read more about this from Andrew and other contributors in issue #9909.
With that in mind "adjusting the code within Cova" has to involve either sharing a pointer to the allocator amongst the subordinate Argument Types or meticulously passing the allocator to dozens of function calls throughout the library that shouldn't require an allocator by default. In this case, the former allows for better separation of concerns in my opinion.
Ultimately, this system does work, but I do understand your concern. Coming from more of an OOP background, it took me a while to adjust to this and I still find it a bit jarring at times.
I suppose it's fine that it's so available I guess. The only thing I can think of is to make a global variable that's only accessible within the library and assignable via a public function, but that's a bad solution to what is probably a non-issue. Let me know how you choose to resolve this!
Looks to me like this is working correctly!