-
Notifications
You must be signed in to change notification settings - Fork 20
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
implement a posixly correct getopt #21
base: main
Are you sure you want to change the base?
Conversation
inc/posix/private/getopt.h
Outdated
#ifndef _PRIVATE_GETOPT_H | ||
#define _PRIVATE_GETOPT_H | ||
|
||
struct z_getopt_r_context { | ||
char *optarg; | ||
int opterr; | ||
int optind; | ||
int optopt; | ||
int optcur; | ||
}; | ||
|
||
int z_getopt_r(struct z_getopt_r_context *context, int argc, char *const argv[], const char *optstring); | ||
|
||
extern char *optarg; | ||
extern int opterr, optind, optopt; | ||
int getopt(int, char * const [], const char *); | ||
int getopt(int argc, char *const argv[], const char *optstring); | ||
|
||
#endif /* _PRIVATE_GETOPT_H */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these seems like private implementation details, I don't think we want or need any of these changes in the public libc headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optcur
is an implementation detail, while optarg
, opterr
, optind
, optopt
are used as specified. if you have a suggestion to hide this implementation detail, feel free to tell me :)
parameter names are useful for auto-completion and the like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameter names seem fine, why are we exposing the z_getopt_r_context
and z_getopt_r
symbols? Are these part of libc? If not, why are they in the libc headers? Why are we exporting them?
src/posix.zig
Outdated
}; | ||
|
||
export fn z_getopt_r(context: *GetoptContext, argc: c_int, argv: [*]const ?[*:0]const u8, optstring: [*:0]const u8) callconv(.C) c_int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this being exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to make a reentrant version available to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is z_getopt_r
a part of libc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it should be exposed to the user as a normal part of libc. perhaps omitting the z_
prefix identifying it as a ziglibc extension would be good, but it's not my choice to make
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it should be exposed to the user as a normal part of libc
What I'm asking is, which standard libc is this function apart of? The cstd, glibc, musl, msvc. The purpose of ziglibc is to provide an alternative implementation for existing libc libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not a part of any other libc, just a reentrant version of the posix function. if providing reentrant versions of functions is out of scope, it shouldn't be exposed.
I've also added some inline tests, not quite sure how to run those eventually.