Re: yes: rearrange code to prevent gcc10 warning

[Christophe Meyering]

That's even better, thank you!

On Thu, Jan 30, 2020 at 5:05 AM Pádraig Brady <address@hidden> wrote:

> On 30/01/2020 07:27, Christophe Meyering wrote:
> > Hello,
> >
> > Using GCC10 on Fedora 31, I built coreutils from a git clone of the
> latest
> > sources.
> > After running
> > $./bootstrap && ./configure && make
> > I got a few deprecation warnings for sys/sysctl.h which I skipped over,
> and
> > found an interesting error while building src/yes.o:
> >    CC       src/yes.o
> > src/yes.c: In function 'main':
> > src/yes.c:110:20: error: writing 1 byte into a region of size 0
> > [-Werror=stringop-overflow=]
> >    110 |   buf[bufused - 1] = '\n';
> >        |   ~~~~~~~~~~~~~~~~~^~~~~~
> > src/yes.c:100:51: note: at offset -1 to an object with size 8192
> allocated
> > by 'xmalloc' here
> >    100 |   char *buf = reuse_operand_strings ? *operands : xmalloc
> > (bufalloc);
> >        |
>  ^~~~~~~~~~~~~~~~~~
> >
> > The compiler didn't deduce that the for loop will always iterate at least
> > once, therefore my first thought was to assert(operands < operand_lim)
> > before the start of the for loop on line 102.
> > I 'heard' :) about assure and decided to use that instead, before
> realizing
> > that I could make it obvious that the loop would run at least once by
> > converting the for loop into a do-while loop.
> > This avoided the warning. I also made sure the tests still pass on F31.
> >
> > Chris Meyering
> >
>
> Very nice.
> Yes it's best to avoid warnings with standard language constructs where
> possible.
> You've changed the loop structure from
> "check", "run", "inc" to "run", "inc", "check",
> which looks perfect.
>
> I wonder would it be better to use the same construct in both loops.
> Is it OK if I roll the following into your patch?
>
> thanks!
> Pádraig
>
> diff --git a/src/yes.c b/src/yes.c
> index 335a23b..c357555 100644
> --- a/src/yes.c
> +++ b/src/yes.c
> @@ -79,7 +79,8 @@ main (int argc, char **argv)
>        large overhead of stdio buffering each item.  */
>     size_t bufalloc = 0;
>     bool reuse_operand_strings = true;
> -  for (char **operandp = operands; operandp < operand_lim; operandp++)
> +  char **operandp = operands;
> +  do
>       {
>         size_t operand_len = strlen (*operandp);
>         bufalloc += operand_len + 1;
> @@ -87,6 +88,7 @@ main (int argc, char **argv)
>             && *operandp + operand_len + 1 != operandp[1])
>           reuse_operand_strings = false;
>       }
> +  while (++operandp < operand_lim);
>
>     /* Improve performance by using a buffer size greater than BUFSIZ /
> 2.  */
>     if (bufalloc <= BUFSIZ / 2)
> @@ -99,7 +101,7 @@ main (int argc, char **argv)
>        the operands strings; this wins when the buffer would be large.  */
>     char *buf = reuse_operand_strings ? *operands : xmalloc (bufalloc);
>     size_t bufused = 0;
> -  char **operandp = operands;
> +  operandp = operands;
>     do
>       {
>         size_t operand_len = strlen (*operandp);
>