Details
Diff Detail
Event Timeline
I wanted to get this out fairly early because there are quite a few things that I wanted to get your input on hdr-gen and policy questions etc
libc/spec/posix.td | ||
---|---|---|
586 | This is a bit of a sad situation. Do you have any thoughts on this, @sivachandra | |
592 | I've just remembered I need to add ObjectSpec's for the global objects. I'll do that in the next round | |
libc/src/unistd/getopt.cpp | ||
108 | How should we be pulling in stdio bits? It seems like we've been trying to make an effort to call other __llvm_libc:: entrypoints from each other. Though fprintf is sufficiently big that I think we would have to just do this. Alternatively we could just use fprintf and that could be either the system fprintf or happen to be llvm-libc's if thats linked in. I'm not sure what our policy is here. | |
libc/src/unistd/getopt.h | ||
17 | Currently the only prior art for extern symbols like this is std{in,out,err}. Though I have some reservations on how those are implemented/tested. Functions being in the __llvm_libc namespace for testing is really useful and I think we should be able to do the same for global objects with aliases just like for functions to ensure hermeticity. I wanted to get your opinion on if you had tried this before. |
libc/spec/posix.td | ||
---|---|---|
586 | The trick would be to declare an internal type like: typedef char *const __getopt_argv_type[]; In the tablegen files, define a NamedType for it and add a type header like this: https://github.com/llvm/llvm-project/blob/main/libc/include/llvm-libc-types/__atexithandler_t.h | |
592 | SGTM | |
libc/src/unistd/getopt.cpp | ||
108 | Can we extend GetoptState [1] with an errstream field which:
[1] - May be we should give GetoptState a better name. | |
libc/src/unistd/getopt.h | ||
17 | I think the examples of stdin and stdout are inappropriate for future global state variables. I want to suggest the following:
void set_getopt_state(char **optarg, int *optind, int *optopt, int optpos, int opterr);
namespace impl { extern "C" char *optarg; extern "C" int optind = 1; extern "C" int optopt; extern "C" int opterr; extern "C" int optpos = 0; } struct GetoptState { char **optarg; int *optind; int *optopt; unsigned *optpos; int opterr; }; // The default state static GetoptState state{&impl::optarg, &impl::optind, &impl::optopt, &impl::optpos, impl::opterr}; void set_getopt_state(char **optarg, int *optind, int *optopt, int *optpos, int opterr) { state.optarg = optarg; state.optind = optind; state.optopt = optopt; state.optpos = optpos; state.opterr = opterr; } The high level goal is to let tests specify where the state of getopt is to be stored by calling set_getopt_state. We can choose to exclude that function under LLVM_LIBC_PUBLIC_PACKAGING. |
libc/spec/posix.td | ||
---|---|---|
586 | Ah that's right. I forgot about that. Thanks | |
libc/src/unistd/getopt.cpp | ||
108 | I've named it GetoptContext, which might be a better name. I've used just stderr instead of __llvm_libc::stderr and same with fprintf. We were hoping to use this on Windows, actually, and pulling in llvm-libc's stdio isn't ready yet in that context. Likewise we are likely to pull this into Fuchsia's libc before stdio too. It would be helpful if we could do with stdio here like we do with malloc elsewhere. Is that okay? |
Still a few comments about the high-level structuring. Once these are sorted out, I will do a pass over the parsing logic.
libc/src/unistd/getopt.cpp | ||
---|---|---|
108 | This problem is likely not unique to getopt. WDYT about a build flag LIBC_USE_EXTERNAL_STDIO which gets converted to compile option -DUSE_EXTERNAL_STDIO. In the source code, when USE_EXTERNAL_STDIO is defined, we use public names to refer to symbols. By default, we use __llvm_libc::<name>. | |
libc/src/unistd/getopt.h | ||
17 | Do we still need the extern declarations? | |
libc/test/src/unistd/getopt_test.cpp | ||
19 | This can go into the implementation header. Any reason not to put it there? | |
60 | This is going to make a lot of things a little messy - I am fine with your code here, but I am just thinking about how this is going to work in the FULL_BUILD mode. May be tests can also look at the LIBC_USE_EXTERNAL_STDIO flag and conditionally add the appropriate DEPENDS. In the source code: #ifdef USE_EXTERNAL_STDIO define FOPENCOOKIE fopencookie ... #else #include "src/stdio/fopencookie.h" define FOPENCOOKIE __llvm_libc::fopencookie ... #endif | |
68 | reset_errstream? | |
69 | get_error_msg? |
LGTM with nits.
libc/src/unistd/getopt.cpp | ||
---|---|---|
17 | Add a comment here saying that this implementation currently does not support GNU extensions. | |
40 | report_error | |
148 | Subjective comment - ISTM that I could have followed this logic readily if it was arranged this way: ++ctx.optind; if (ctx.optind >= argc ...) { } ctx.optarg.get() = argv[ctx.optind]; | |
libc/test/src/unistd/getopt_test.cpp | ||
115 | We actually follow this style: int('?') Better may be, like you have added the _c suffix, add _i suffix. |
Going to let the premerge run before pushing, because it wasn't working without full build before
libc/src/unistd/getopt.cpp | ||
---|---|---|
148 | I've added a comment to explain why I prefer to keep it as is, because optind should stay at the previous value upon return |
This is a bit of a sad situation. Do you have any thoughts on this, @sivachandra