Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints

[Paul Eggert]

On 12/1/21 13:02, Robbie Harwood wrote:

> diff --git a/lib/argp-fmtstream.c b/lib/argp-fmtstream.c
> index f679751b9..4aa401e2d 100644
> --- a/lib/argp-fmtstream.c
> +++ b/lib/argp-fmtstream.c
> @@ -234,7 +234,7 @@ __argp_fmtstream_update (argp_fmtstream_t fs)
>         else
>         {
>           size_t display_width = mbsnwidth (buf, nl - buf, MBSW_STOP_AT_NUL);
> -         if (display_width < (ssize_t) fs->rmargin)
> +         if (display_width < fs->rmargin)
>             {
>               /* The buffer contains a full line that fits within the maximum
>                  line width.  Reset point and scan the next line.  */

This fixes a problem introduced in PATCH v2 04/10 "Fix width 
computation". Please fix that patch instead.


> -      Idx node = init_state->nodes.elems[node_cnt];
> +      size_t node = init_state->nodes.elems[node_cnt];

Let's not make this sort of change. We are moving to a coding style that 
prefers signed values, since they allow us to use better runtime checks. 
If -Wsign-compare complains, let's disable -Wsign-compare instead of 
changing carefully-written signed integers to be unsigned.


> -             if (__mbrtowc (&wc, (const char *) buf, p - buf,
> +             if ((ssize_t)__mbrtowc (&wc, (const char *) buf, p - buf,
>                              &state) == p - buf

Let's leave this alone as well. The code is OK as-is, and I'd rather not 
insert casts (they're too powerful and error-prone in C).

> @@ -860,7 +861,8 @@ init_dfa (re_dfa_t *dfa, size_t pat_len)
>         dfa->sb_char = (re_bitset_ptr_t) utf8_sb_map;
>         else
>         {
> -         int i, j, ch;
> +         int i, j;
> +          wint_t ch;

ch can be at most 65536 so 'int' should be OK here and it's better to 
use a signed integer as mentioned earlier.


The remaining changes in this patch also seem to be unnecessary; they're 
put in only to pacify -Wsign-compare, so the solution is to not use 
-Wsign-compare. That's what Coreutils does, for example.