[U-Boot] [PATCH 3/3] sandbox: add getopt support

[Simon Glass]

Hi Mike,

On Sun, Feb 26, 2012 at 8:35 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Sunday 26 February 2012 23:26:52 Simon Glass wrote:
>> On Sun, Feb 26, 2012 at 8:20 PM, Mike Frysinger wrote:
>> > On Sunday 26 February 2012 21:46:23 Simon Glass wrote:
>> >> On Sun, Feb 26, 2012 at 2:38 PM, Mike Frysinger wrote:
>> >> >  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.
>
> my plan was to clean this up a bit more before submitting (such as adding
> comments).  i was looking more for feedback on the general approach here and
> any fundamental sticking points since this is a semi-radical departure from
> what either of us posted earlier.

OK I see. Well actually I was expecting that we would need something
along these lines eventually, since arg parsing belongs with the
module that provides the argument I think. So as well to have it now,
even if it doesn't have a lot of uses yet. It will.

>
> in this case, i'm asking: should we just toss the return value and have it be
> void ?

I suggest not - even if we ignore it, it seems like information that
should be returned. Perhaps we should make it more explicit by
returning state->return_code with a comment that callers can use it
now or later. But I think the way you have done it is fine.

Regards,
Simon

> -mike