Re: [PATCH] Test command

[Yoshinori K. Okuji]

On Sunday 12 April 2009 00:11:45 phcoder wrote:
> Updated. Same changelog
>
> >> +       {
> >> +         update_val (grub_strcmp (args[*argn], args[*argn + 2]) == 0);
> >> +         (*argn) += 3;
> >
> > I myself feel that these parentheses are redundant, but I don't know how
> > others think. For C programmers, it is well known that * has a very high
> > priority.
>
> These parenthesis are necessary if doing sth like
> (*argn)++
> since ++ and += are logically and visually similar I prefer to put the
> parenthesis

OK.

>
> > Getting tired, so I skip the same criticism from here.
>
> Actually it would have been enough to say "same applies further on in
> patch"
>
> >> +      if (*argn + 1 < argc && !grub_strcmp (args[*argn], "-s"))
> >> +       {
> >> +         grub_file_t file;
> >> +         file = grub_file_open (args[*argn + 1]);
> >> +         update_val (file && grub_file_size (file));
> >
> > This is not very safe, because grub_file_size returns grub_off_t which is
> > a 64-bit unsigned int. By converting it into 32-bit signed int
> > implicitly, the result can be zero, even when the size is not zero. So it
> > is better to say explicitly, != 0.


BTW, I think you can simplify test_parse. For example, you write "if (*argn + 
2 < argc ...)" many times, but it should be possible to test this condition 
only once per loop.

Regards,
Okuji