Hi Mike,
On Sun, Feb 26, 2012 at 8:20 PM, Mike Frysinger <vap...@gentoo.org> wrote:
> On Sunday 26 February 2012 21:46:23 Simon Glass wrote:
>> On Sun, Feb 26, 2012 at 2:38 PM, Mike Frysinger wrote:
>> > --- a/arch/sandbox/cpu/os.c
>> > +++ b/arch/sandbox/cpu/os.c
>> >
>> > +extern struct sb_cmdline_option *__u_boot_sb_getopt_start[],
>> > + *__u_boot_sb_getopt_end[];
>>
>> I wonder if we can put this in asm-generic/sections.h - or perhaps
>> that doesn't exist yet?
>
> sorry, should have labeled this patch more as a PoC as i know it requires a
> little more polish. these would go into sandbox's asm/sections.h since these
> are specific to the sandbox port.
>
>> Also how about __u_boot_sandbox_option_start/end? I'm really not keen on
>> 'sb'.
>
> for these two vars, that's fine. i prefer "sb" for internal static stuff
> since
> "sandbox" can get wordy real fast.
>
>> > + int hidden_short_opt;
>> > + size_t si;
>>
>> si?
>
> short_opt_index is the self-commenting name
>
>> > + short_opts = os_malloc(sizeof(*short_opts) * (num_flags + 1));
>>
>> Comment on why +1? is it for \0 terminator?
>
> yes, the getopt_long() api requires a NUL terminated string
>
>> > + si = 0;
>> > + hidden_short_opt = 0x80;
>> > + for (i = 0; i < num_flags; ++i) {
>> > + long_opts[i].name = sb_opt[i]->flag;
>> > + long_opts[i].has_arg = sb_opt[i]->has_arg ?
>> > + required_argument : no_argument;
>> > + long_opts[i].flag = NULL;
>> > +
>> > + if (sb_opt[i]->flag_short)
>> > + short_opts[si++] = long_opts[i].val =
>> > sb_opt[i]->flag_short; + else
>> > + long_opts[i].val = sb_opt[i]->flag_short =
>> > hidden_short_opt++;
>>
>> What is this hidden_short_opt for? Suggest a comment.
>
> i think most options we have will be long only. much harder to make sure you
> don't have collisions in the entire base when the flag definition is in the
> subfiles. but getopt_long() needs a unique int for each long flag, so
> "hidden"
> just means "not an ascii char".
>
>> > + if (state->parse_err < 0)
>> > + printf("error: unknown option: %s\n\n",
>> > + state->argv[-state->parse_err - 1]);
>> > +
>> > + printf(
>>
>> do we need this extra newline?
>
> i find the extra newline helps to differentiate from the noise when we turn
> around and dump the --help output. alternative would be to not automatically
> show --help.
>
>> > + for (i = 0; i < num_flags; ++i) {
>> > + /* first output the short flag if it has one */
>> > + if (sb_opt[i]->flag_short >= 0x80)
>>
>> Can we declare a pointer to sb_opt[i] and the top here and use it below?
>
> yes
>
>> > + printf(" ");
>> > + else
>> > + printf(" -%c, ", sb_opt[i]->flag_short);
>> > +
>> > + /* then the long flag */
>> > + if (sb_opt[i]->has_arg)
>> > + printf("--%-*s", max_noarg_len, sb_opt[i]->flag);
>> > + else
>> > + printf("--%-*s <arg> ", max_arg_len,
>> > sb_opt[i]->flag); +
>> > + /* finally the help text */
>> > + printf(" %s\n", sb_opt[i]->help);
>>
>> puts() might be safer, but then again I think we have vsnprintf() turned on
>> now.
>
> not sure what you mean by "safer" ... if you mean the implicit stack
> overflows,
> i don't think that'll be an issue here. the help strings aren't very long.
>
>> > int main(int argc, char *argv[])
>> > {
>> > ...
>> > + state = state_get_current();
>> > + os_parse_args(state, argc, argv);
>>
>> We don't check the return value. Perhaps add a comment as to why.
>
> since the code takes care of setting parse_err itself now, i'm not sure what
> to do with the return value
I agree it is right, just asking for a comment. Same with most of my
other things - more comments as I suggested is nice for people that
come into the code fresh.
Regards,
Simon
> -mike
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
