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

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

[Philip Oakley]

Duy,

In 
https://github.com/git-for-windows/git/commit/b3c96fb158f05152336f167076f5d81d23c3a5e5
(split-index: strip pathname of on-disk replaced entries, 13 Jun 2014) 
Nguyễn Thái Ngọc Duy <pclouds@gmail.com>  you have

read-cache.c#L1790 (in that commit, now ~L1873)

    int saved_namelen = saved_namelen; /* compiler workaround */

Which then becomes an MSVC compile warning C4700: uninitialized local 
variable.

I'm wondering what was the compiler workaround being referred to? i.e. why 
does it need that tweak? There's no mention of the reason in the commit 
message.

There are a few other occurences of this same initialisation issue, so I was 
wondering if it's still neded? Or should I just get MSVC to ignore it, which 
may hide other issues..

--
Philip

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

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

>     int saved_namelen = saved_namelen; /* compiler workaround */
>
> Which then becomes an MSVC compile warning C4700: uninitialized local 
> variable.
>
> I'm wondering what was the compiler workaround being referred to? i.e. why 
> does it need that tweak? There's no mention of the reason in the commit 
> message.

That was a fairly well-known workaround for GCC that issues a false
warning that variable is used before initialized.  I thought we
stopped using it several years ago in new code after doing a bulk
sanitizing (I think the new recommended workaround was to initialise
such a variable to the nil value like '0' for integers).

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

[Duy Nguyen]

On Fri, May 6, 2016 at 4:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>>     int saved_namelen = saved_namelen; /* compiler workaround */
>>
>> Which then becomes an MSVC compile warning C4700: uninitialized local
>> variable.
>>
>> I'm wondering what was the compiler workaround being referred to? i.e. why
>> does it need that tweak? There's no mention of the reason in the commit
>> message.
>
> That was a fairly well-known workaround for GCC that issues a false
> warning that variable is used before initialized.  I thought we
> stopped using it several years ago in new code after doing a bulk
> sanitizing

I guess that's 803a777 (cat-file: Fix an gcc -Wuninitialized warning -
2013-03-26) and more commits around that time. The split-index commit
is in 2014. I must have missed the trend.

> (I think the new recommended workaround was to initialise
> such a variable to the nil value like '0' for integers).

Yep. First Jeff removed the " = xxx" part from "xxx = xxx" then Ramsay
added the " = NULL" back. So we probably just do "int saved_namelen =
0;" in this case.
-- 
Duy

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

[Philip Oakley]

From: "Duy Nguyen" <pclouds@gmail.com>
> On Fri, May 6, 2016 at 4:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> "Philip Oakley" <philipoakley@iee.org> writes:
>>
>>>     int saved_namelen = saved_namelen; /* compiler workaround */
>>>
>>> Which then becomes an MSVC compile warning C4700: uninitialized local
>>> variable.
>>>
>>> I'm wondering what was the compiler workaround being referred to? i.e. 
>>> why
>>> does it need that tweak? There's no mention of the reason in the commit
>>> message.
>>
>> That was a fairly well-known workaround for GCC that issues a false
>> warning that variable is used before initialized.  I thought we
>> stopped using it several years ago in new code after doing a bulk
>> sanitizing
>
> I guess that's 803a777 (cat-file: Fix an gcc -Wuninitialized warning -
> 2013-03-26) and more commits around that time. The split-index commit
> is in 2014. I must have missed the trend.
>
>> (I think the new recommended workaround was to initialise
>> such a variable to the nil value like '0' for integers).
>
> Yep. First Jeff removed the " = xxx" part from "xxx = xxx" then Ramsay
> added the " = NULL" back. So we probably just do "int saved_namelen =
> 0;" in this case.
> -- 
Thanks,

I'll try and work up a patch - probably next week as I'm away for the 
weekend.

--
Philip 

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

[Ramsay Jones]

On 06/05/16 14:15, Philip Oakley wrote:
> From: "Duy Nguyen" <pclouds@gmail.com>
>> On Fri, May 6, 2016 at 4:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> "Philip Oakley" <philipoakley@iee.org> writes:
>>>
>>>>     int saved_namelen = saved_namelen; /* compiler workaround */
>>>>
>>>> Which then becomes an MSVC compile warning C4700: uninitialized local
>>>> variable.
>>>>
>>>> I'm wondering what was the compiler workaround being referred to? i.e. why
>>>> does it need that tweak? There's no mention of the reason in the commit
>>>> message.
>>>
>>> That was a fairly well-known workaround for GCC that issues a false
>>> warning that variable is used before initialized.  I thought we
>>> stopped using it several years ago in new code after doing a bulk
>>> sanitizing
>>
>> I guess that's 803a777 (cat-file: Fix an gcc -Wuninitialized warning -
>> 2013-03-26) and more commits around that time. The split-index commit
>> is in 2014. I must have missed the trend.
>>
>>> (I think the new recommended workaround was to initialise
>>> such a variable to the nil value like '0' for integers).
>>
>> Yep. First Jeff removed the " = xxx" part from "xxx = xxx" then Ramsay
>> added the " = NULL" back. So we probably just do "int saved_namelen =
>> 0;" in this case.
>> -- 
> Thanks,
> 
> I'll try and work up a patch - probably next week as I'm away for the weekend.

Yeah, I don't remember why these were left over from the previous
attempt to clean these up (maybe they conflicted with in-flight
topics?), but I have had a patch hanging around ... :-D

The patch below applies to master (I haven't checked for any more
additions).

ATB,
Ramsay Jones

-- >8 --
From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Subject: [PATCH] -Wuninitialized: remove a gcc specific workaround

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 builtin/rev-list.c | 2 +-
 fast-import.c      | 4 ++--
 merge-recursive.c  | 2 +-
 read-cache.c       | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 275da0d..deae1f3 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 = reaches, all = all;
+		int reaches = 0, all = 0;
 
 		revs.commits = find_bisection(revs.commits, &reaches, &all,
 					      bisect_find_all);
diff --git a/fast-import.c b/fast-import.c
index 9fc7093..ca66d80 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2935,7 +2935,7 @@ static void cat_blob(struct object_entry *oe, unsigned char sha1[20])
 
 static void parse_get_mark(const char *p)
 {
-	struct object_entry *oe = oe;
+	struct object_entry *oe = NULL;
 	char output[42];
 
 	/* get-mark SP <object> LF */
@@ -2952,7 +2952,7 @@ static void parse_get_mark(const char *p)
 
 static void parse_cat_blob(const char *p)
 {
-	struct object_entry *oe = oe;
+	struct object_entry *oe = NULL;
 	unsigned char sha1[20];
 
 	/* cat-blob SP <object> LF */
diff --git a/merge-recursive.c b/merge-recursive.c
index 06d31ed..9cecc24 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1897,7 +1897,7 @@ int merge_recursive(struct merge_options *o,
 {
 	struct commit_list *iter;
 	struct commit *merged_common_ancestors;
-	struct tree *mrtree = mrtree;
+	struct tree *mrtree = NULL;
 	int clean;
 
 	if (show(o, 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;
 	char *name;
 	int result;
 
-- 
2.8.0

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

[Philip Oakley]

From: "Ramsay Jones" <ramsay@ramsayjones.plus.com>
> On 06/05/16 14:15, Philip Oakley wrote:
>> From: "Duy Nguyen" <pclouds@gmail.com>
>>> On Fri, May 6, 2016 at 4:41 AM, Junio C Hamano <gitster@pobox.com> 
>>> wrote:
>>>> "Philip Oakley" <philipoakley@iee.org> writes:
>>>>
>>>>>     int saved_namelen = saved_namelen; /* compiler workaround */
>>>>>
>>>>> Which then becomes an MSVC compile warning C4700: uninitialized local
>>>>> variable.
>>>>>
>>>>> I'm wondering what was the compiler workaround being referred to? i.e. 
>>>>> why
>>>>> does it need that tweak? There's no mention of the reason in the 
>>>>> commit
>>>>> message.
>>>>
>>>> That was a fairly well-known workaround for GCC that issues a false
>>>> warning that variable is used before initialized.  I thought we
>>>> stopped using it several years ago in new code after doing a bulk
>>>> sanitizing
>>>
>>> I guess that's 803a777 (cat-file: Fix an gcc -Wuninitialized warning -
>>> 2013-03-26) and more commits around that time. The split-index commit
>>> is in 2014. I must have missed the trend.
>>>
>>>> (I think the new recommended workaround was to initialise
>>>> such a variable to the nil value like '0' for integers).
>>>
>>> Yep. First Jeff removed the " = xxx" part from "xxx = xxx" then Ramsay
>>> added the " = NULL" back. So we probably just do "int saved_namelen =
>>> 0;" in this case.
>>> -- 
>> Thanks,
>>
>> I'll try and work up a patch - probably next week as I'm away for the 
>> weekend.
>
> Yeah, I don't remember why these were left over from the previous
> attempt to clean these up (maybe they conflicted with in-flight
> topics?), but I have had a patch hanging around ... :-D
>
> The patch below applies to master (I haven't checked for any more
> additions).

Looks good to me. Catches those I've seen.

Have a good weekend all.

>
> ATB,
> Ramsay Jones
>
> -- >8 --
> From: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Subject: [PATCH] -Wuninitialized: remove a gcc specific workaround
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
> builtin/rev-list.c | 2 +-
> fast-import.c      | 4 ++--
> merge-recursive.c  | 2 +-
> read-cache.c       | 2 +-
> 4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 275da0d..deae1f3 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 = reaches, all = all;
> + int reaches = 0, all = 0;
>
>  revs.commits = find_bisection(revs.commits, &reaches, &all,
>        bisect_find_all);
> diff --git a/fast-import.c b/fast-import.c
> index 9fc7093..ca66d80 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2935,7 +2935,7 @@ static void cat_blob(struct object_entry *oe, 
> unsigned char sha1[20])
>
> static void parse_get_mark(const char *p)
> {
> - struct object_entry *oe = oe;
> + struct object_entry *oe = NULL;
>  char output[42];
>
>  /* get-mark SP <object> LF */
> @@ -2952,7 +2952,7 @@ static void parse_get_mark(const char *p)
>
> static void parse_cat_blob(const char *p)
> {
> - struct object_entry *oe = oe;
> + struct object_entry *oe = NULL;
>  unsigned char sha1[20];
>
>  /* cat-blob SP <object> LF */
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 06d31ed..9cecc24 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1897,7 +1897,7 @@ int merge_recursive(struct merge_options *o,
> {
>  struct commit_list *iter;
>  struct commit *merged_common_ancestors;
> - struct tree *mrtree = mrtree;
> + struct tree *mrtree = NULL;
>  int clean;
>
>  if (show(o, 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;
>  char *name;
>  int result;
>
> -- 
> 2.8.0
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Junio C Hamano]

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.

> 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?

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

[Marc Branchaud]

On 2016-05-06 02:54 PM, Junio C Hamano wrote:
>
> 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?

Perhaps

	#define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0

or, for short-and-sweet

	#define CUWI 0

?

:)

		M.

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

[Junio C Hamano]

Marc Branchaud <marcnarc@xiplink.com> writes:

> On 2016-05-06 02:54 PM, Junio C Hamano wrote:
>>
>> 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?
>
> Perhaps
>
> 	#define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0
>
> or, for short-and-sweet
>
> 	#define CUWI 0
>
> ?
>
> :)

I get that smiley.

I was hinting to keep the /* compiler workaround */ comment, but
in a bit shorter form.

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

[Stefan Beller]

On Fri, May 6, 2016 at 12:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Marc Branchaud <marcnarc@xiplink.com> writes:
>
>> On 2016-05-06 02:54 PM, Junio C Hamano wrote:
>>>
>>> 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?
>>
>> Perhaps
>>
>>       #define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0
>>
>> or, for short-and-sweet
>>

           /* Here could go a longer explanation than the 4 character
define :) */
>>       #define CUWI 0
>>
>> ?
>>
>> :)
>
> I get that smiley.
>
> I was hinting to keep the /* compiler workaround */ comment, but
> in a bit shorter form.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ramsay Jones]

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

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

[Marc Branchaud]

On 2016-05-06 03:57 PM, Junio C Hamano wrote:
> Marc Branchaud <marcnarc@xiplink.com> writes:
>
>> On 2016-05-06 02:54 PM, Junio C Hamano wrote:
>>>
>>> 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?
>>
>> Perhaps
>>
>> 	#define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0
>>
>> or, for short-and-sweet
>>
>> 	#define CUWI 0
>>
>> ?
>>
>> :)
>
> I get that smiley.

Of course, right after I sent that I thought of

#define SPURIOUS_COMPILER_RELATED_UNINITIALIZED_WARNING_INITIALIZER 0

or

#define SCRUWI 0

Which we'd get to pronounce as "screwy".

OK, I'll shut up now.

		M.

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

[Ramsay Jones]

On 06/05/16 21:21, Ramsay Jones wrote:
> On 06/05/16 19:54, Junio C Hamano wrote:
>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>>
[snip]

> 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)
>             ^
>   $ 

BTW, another patch that I have hanging around ... this time the
patch below was built on master from a couple of years ago (so,
I haven't bothered to rebase it, but you should get the idea):

ATB,
Ramsay Jones

-- >8 --
Subject: [PATCH] git-compat-util.h: xsize_t(): avoid signed comparison warnings

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 git-compat-util.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index c07e0c1..3a9cf6c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -838,9 +838,10 @@ static inline char *xstrdup_or_null(const char *str)
 
 static inline size_t xsize_t(off_t len)
 {
-	if (len > (size_t) len)
+	size_t r = (size_t)len;
+	if (len != (off_t)r)
 		die("Cannot handle files this big");
-	return (size_t)len;
+	return r;
 }
 
 __attribute__((format (printf, 3, 4)))
-- 
2.8.0

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

[Philip Oakley]

From: "Stefan Beller" <sbeller@google.com>
> On Fri, May 6, 2016 at 12:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Marc Branchaud <marcnarc@xiplink.com> writes:
>>
>>> On 2016-05-06 02:54 PM, Junio C Hamano wrote:
>>>>
>>>> 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?
>>>
>>> Perhaps
>>>
>>>       #define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0
>>>
>>> or, for short-and-sweet
>>>
>
>           /* Here could go a longer explanation than the 4 character
> define :) */
>>>       #define CUWI 0
>>>
>>> ?
>>>
>>> :)
>>
>> I get that smiley.
>>
>> I was hinting to keep the /* compiler workaround */ comment, but
>> in a bit shorter form.
>> --

For some background, I found $gmane/169098/focus=169104 which covers some of 
the issues (the focused msg is one from Junio). Hannes then notes 
($gmane/169121) that the current xx = xx; could be seen as possible 
undefined behaviour - perhaps one of those 'no good solution' problems.

Perhaps a suitable name...

#define SUPPRESS_COMPILER_UNINITIALIZED_WARNING 0
/* Use when some in-use compiler is unable to determine if the variable is 
used uninitialized, and no good default value is available */

Though, where best to put it?

--
Philip 

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

[Randall S. Becker]

On May 9, 2016 3:40 PM Philip Oakley wrote:
> From: "Stefan Beller" <sbeller@google.com>
> > On Fri, May 6, 2016 at 12:57 PM, Junio C Hamano <gitster@pobox.com>
> wrote:
> >> Marc Branchaud <marcnarc@xiplink.com> writes:
> >>
> >>> On 2016-05-06 02:54 PM, Junio C Hamano wrote:
> >>>>
> >>>> 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?
> >>>
> >>> Perhaps
> >>>
> >>>       #define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0
> >>>
> >>> or, for short-and-sweet
> >>>
> >
> >           /* Here could go a longer explanation than the 4 character
> > define :) */
> >>>       #define CUWI 0
> >>>
> >>> ?
> >>>
> >>> :)
> >>
> >> I get that smiley.
> >>
> >> I was hinting to keep the /* compiler workaround */ comment, but in a
> >> bit shorter form.
> >> --
> 
> For some background, I found $gmane/169098/focus=169104 which covers
> some of the issues (the focused msg is one from Junio). Hannes then notes
> ($gmane/169121) that the current xx = xx; could be seen as possible
> undefined behaviour - perhaps one of those 'no good solution' problems.
> 
> Perhaps a suitable name...
> 
> #define SUPPRESS_COMPILER_UNINITIALIZED_WARNING 0
> /* Use when some in-use compiler is unable to determine if the variable is
> used uninitialized, and no good default value is available */
> 
> Though, where best to put it?

I would suggest this type of approach should be detected in the configure script and added automagically (as best as possible) or as a hint supplied by the platform's own specific configuration files if necessary as a last gasp.

-- Brief whoami: NonStop&UNIX developer since approximately UNIX(421664400)/NonStop(211288444200000000)
-- In my real life, I talk too much.