Bug#625357: jigit: ftbfs with gcc-4.6 -Werror

[Steve McIntyre <steve@einval.com>]

On Thu, Jun 02, 2011 at 01:16:59PM -0500, Jonathan Nieder wrote:
>severity 625357 normal
>retitle 625357 gcc -Wunused-but-set-variable should not be implied by -Wall (?)
>tags 625357 = upstream moreinfo
>quit
>
>Hi again,
>
>Steve McIntyre wrote:
>
>> I'll remove the -Werror to stop gcc breaking the build here, but I
>> definitely believe that gcc is doing the wrong thing here.
>> Technically, yes - the variables are set but unused. However, this is
>> a far higher level of pedantry than is warranted for -Wall. The code
>> being complained about is perfectly valid, typical of the defensive
>> programming pattern of "initialise all variables".
>
>To clarify: here's a demo of the behavior of gcc-4.4.
>
> $ program_one='int main(void) { int x = 5; return 0; }'
> $ program_two='int main(void) { int x; x = 5; return 0; }'
> $ echo "$program_one" | gcc-4.4 -Wall -x c -
> <stdin>: In function ‘main’:
> <stdin>:1: warning: unused variable ‘x’
> $ echo "$program_two" | gcc-4.4 -Wall -x c -

Sure, that's confused. :-)

>So imho gcc before v4.6 is just fundamentally confused.  Which means
>there are a number of ways one could go with this.  Do you mean:
>
> A. Unused variables are not a big deal, and they belong in -Wextra,
>    not -Wall.

No.

> B. New warnings are a pain in the neck and should go in -Wextra
>    during a transition period.

Maybe, but a lot of people will never see them anyway until they cause
build failures. :-)

> C. The idiom
>
>	ssize_t unused;
>	/*
>	 * Yes, there might be an error, dear gcc -D_FORTIFY_SOURCE,
>	 * but we want to ignore it.
>	 */
>	unused = write(...);
>
>   in place of, say,
>
>	/* loop on partial writes and EINTR */
>	xwrite(...);
>
>   is good style and the -Wunused-but-set-variable warning is
>   fundamentally misguided.

I thought I was looking at a different case, but I've just re-tested
various things to respond to you here. It looks like I've missed
*exactly* the thing gcc was complaining about, and it is case C
here. Now I can see that, I'm less unhappy about the warning.

My code is analogous to your example code:

static int parse_md5_entry(char *md5_entry)
{
    int error = 0;
    char *file_name = NULL;
    char *md5 = NULL;
    unsigned char bin_md5[16];
    int i;

    ...
    /* code that does the real meat of the work to fill in "md5" and
       "filename" , but doesn't set/use "error" at all */
    ...
    
    error = add_md5_entry(UNKNOWN, md5, file_name);
    return 0;
}

To explain a bit more: the warning reported by gcc (unhelpfully)
points at the declaration/initialisation of "error", which led me to
(incorrectly) think that it was complaining about set-but-unused for
the initialisation. With minor tweaks, I can see that it's complaining
about the return value from add_md5_entry() being ignored but doesn't
*say* that. :-(

I'm about to update the code around that area now. More accurate
diagnostics from gcc would be a major improvement, I think!

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
"...In the UNIX world, people tend to interpret `non-technical user'
 as meaning someone who's only ever written one device driver." -- Daniel Pead