Dear Wolfgang,
On Mon, Jun 06, 2011 at 09:16:00PM +0200, Wolfgang Denk wrote:
> > -# ifndef CONFIG_SYS_HUSH_PARSER
> > - run_command (p, 0);
> > -# else
> > - parse_string_outer(p, FLAG_PARSE_SEMICOLON |
> > - FLAG_EXIT_FROM_LOOP);
> > -# endif
> > + run_command2(p, 0);
>
> Indentation looks incorrect here?
Yes, it is incorrect. I will correct it in the next version of the
patch.
>
> > -# endif
> > - }
> > + if (s)
> > + run_command2(s, 0);
>
> Indentation always by TABs please.
The if (s) line and the rest of this block were indented with spaces
before I changed it. Should I make the cosmetic change of correct the
indentation there to use tabs as part of this patch?
> > +int run_command2(const char *cmd, int flag)
> > +{
> > +#ifndef CONFIG_SYS_HUSH_PARSER
> > + if (run_command(cmd, flag) == -1)
> > + return 1;
> > +#else
> > + if (parse_string_outer(cmd,
> > + FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP) != 0)
> > + return 1;
> > +#endif
> > + return 0;
>
> Why do you make this function return int when there is no tesing of
> the return code anywhere?
Where run_command2 replaced run_command/parse_string_outer blocks in
main.c, there was no checking of the return codes of those functions
already. I do use the return code of it in cmd_pxecfg.c, which is
introduced later in this patch series.
> And why do you make this not an inline function (or macro) so the
> compiler could avoid the function call overhead?
No particular reason - I just didn't consider optimization. Is common.h
the best place to place this as an inline function?
Thanks,
Jason
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
