Re: /* compiler workaround */ - what was the issue?
- From
Ramsay Jones <ramsay@ramsayjones.plus.com>
- Date
- May 6, 2016, 20:21 UTC
- Message-ID
- <572CFCBC.3020103@ramsayjones.plus.com>
- In-Reply-To
- <xmqq60ur3tlu.fsf@gitster.mtv.corp.google.com>
On 06/05/16 19:54, Junio C Hamano wrote:
Show 23 quoted lines
> 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]Show 16 quoted lines
>
>> 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