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

Add a std.mem.Allocator argument to the type definition of every custom function parameter? #30

Closed
p7r0x7 opened this issue Oct 17, 2023 · 8 comments
Labels
enhancement New feature or request
Milestone

Comments

@p7r0x7
Copy link

p7r0x7 commented Oct 17, 2023

It doesn't have to be used, but it should be available if necessary.

@00JCIV00
Copy link
Owner

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.

@p7r0x7
Copy link
Author

p7r0x7 commented Oct 17, 2023

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.

@00JCIV00
Copy link
Owner

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!

@00JCIV00 00JCIV00 added the enhancement New feature or request label Oct 17, 2023
@p7r0x7
Copy link
Author

p7r0x7 commented Oct 17, 2023

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.

@00JCIV00
Copy link
Owner

00JCIV00 commented Oct 18, 2023

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.

@p7r0x7
Copy link
Author

p7r0x7 commented Oct 18, 2023

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!

@00JCIV00 00JCIV00 added this to the v0.9.0-beta milestone Oct 20, 2023
00JCIV00 added a commit that referenced this issue Oct 20, 2023
- Added `_alloc` field and `init()` method to Options and Values so the `mem.Allocator` can be passed in from the parent Command during Initialization.
- Added `mem.Allocator` Callback Function Parameter requirement so that all callbacks can use the Argument Type's allocator if needed.
- Updated Docs to reflect these changes.
- Closes #30
@00JCIV00
Copy link
Owner

00JCIV00 commented Oct 20, 2023

@p7r0x7 Whenever you get a chance, checkout this commit (f799141). Allocators are now a required function parameter for all callback functions.

This actually let me include a few additional helper functions under Value.ParsingFns. Good call! Let me know if you run into any issues with it.

00JCIV00 added a commit that referenced this issue Oct 20, 2023
- Allowed Custom `Value.Typed` Types in `Value.Config.custom_types` to make custom Value Types more seamless and configurable.
- Fixed misssing `mem.Allocator` parameter in `Command.Config.usage_fn`.
- #30
00JCIV00 added a commit that referenced this issue Oct 21, 2023
- Allowed arbitrary Types in `Value.Config.custom_types` to overwrite existing Types in `Value.Generic`.
- Added `custom_parse_fns` field into `Value.Config` to allow for customizable Parsing Functions for added or overwritten Types.
- #30
@p7r0x7
Copy link
Author

p7r0x7 commented Oct 21, 2023

Looks to me like this is working correctly!

@p7r0x7 p7r0x7 closed this as completed Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants