Re: /* compiler workaround */ - what was the issue?

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

On 06/05/16 19:54, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> The patch below applies to master (I haven't checked for any more
>> additions).
>>
>>  	if (bisect_list) {
>> -		int reaches = reaches, all = all;
>> +		int reaches = 0, all = 0;
> 
> One thing that is somewhat sad is that this makes future readers
> wonder if these values '0' are sensible initial values.
> 
> Having to wonder "is it sensible to initialize this variable to 0?
> Shouldn't it be initialized to INT_MAX instead?" is wasting their
> time exactly because we _know_ these are not even "initial values".
> We know these do not have to be initialized, because some more
> appropriate values will get assigned to them before they are used,
> and have these only because some compilers get it wrong.
> 
> The original "reaches = reaches" had the documentation value (at
> least for those who knew the convention) to save the readers from
> wasting their time that way.  Now these 0 are indistinguishable from
> the other initializations that require to be zero.

Ah, I think I remember now why I hadn't sent this patch before. ;-)
[This started off as one patch, was then split into two (int and pointer),
and then back into one again - presumably because I had by that time
forgotten why I split it up!]

I have a very vague recollection of you expressing your dislike of
these parts of the patch before. I had intended to investigate why
gcc was incorrectly issuing these warnings - but I couldn't get my
currently installed compiler to complain. That would have meant
building various gcc versions, so that I could bisect ...

So, that's why I didn't get around to it ... :-D

I still can't get gcc to complain, e.g. (on top of above):

  $ git diff
  diff --git a/builtin/rev-list.c b/builtin/rev-list.c
  index deae1f3..845fcdc 100644
  --- a/builtin/rev-list.c
  +++ b/builtin/rev-list.c
  @@ -377,7 +377,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
                  mark_edges_uninteresting(&revs, show_edge);
   
          if (bisect_list) {
  -               int reaches = 0, all = 0;
  +               int reaches, all;
   
                  revs.commits = find_bisection(revs.commits, &reaches, &all,
                                                bisect_find_all);
  $ rm builtin/rev-list.o
  $ make V=1 CFLAGS='-g -O3 -Wall -Wextra -Wuninitialized -Wno-unused-parameter' builtin/rev-list.o
  cc -o builtin/rev-list.o -c -MF builtin/.depend/rev-list.o.d -MQ builtin/rev-list.o -MMD -MP  -g -O3 -Wall -Wextra -Wuninitialized -Wno-unused-parameter -I. -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DXDL_FAST_HASH -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM -DSHA1_HEADER='<openssl/sha.h>'  -DNO_STRLCPY -DNO_MKSTEMPS -DSHELL_PATH='"/bin/sh"'  builtin/rev-list.c
  In file included from ./cache.h:4:0,
                   from builtin/rev-list.c:1:
  ./git-compat-util.h: In function ‘xsize_t’:
  ./git-compat-util.h:838:10: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
    if (len > (size_t) len)
            ^
  $ 
  
[Note: gcc (Ubuntu 4.8.4-2ubuntu1~14.04.1) 4.8.4]

> 
>> diff --git a/read-cache.c b/read-cache.c
>> index d9fb78b..978d6b6 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1870,7 +1870,7 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,
>>  {
>>  	int size;
>>  	struct ondisk_cache_entry *ondisk;
>> -	int saved_namelen = saved_namelen; /* compiler workaround */
>> +	int saved_namelen = 0;
> 
> I wonder if can we come up with a short and sweet notation to remind
> futhre readers that this "initialization" is not initializing but
> merely squelching warnings from stupid compilers, and agree to use
> it consistently?

Nothing comes to mind.

Do current compilers complain?

ATB,
Ramsay Jones