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

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

/* 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 <[hidden email]>  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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
"Philip Oakley" <[hidden email]> 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).


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Duy Nguyen
On Fri, May 6, 2016 at 4:41 AM, Junio C Hamano <[hidden email]> wrote:

> "Philip Oakley" <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Philip Oakley
From: "Duy Nguyen" <[hidden email]>

> On Fri, May 6, 2016 at 4:41 AM, Junio C Hamano <[hidden email]> wrote:
>> "Philip Oakley" <[hidden email]> 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Ramsay Jones-2


On 06/05/16 14:15, Philip Oakley wrote:

> From: "Duy Nguyen" <[hidden email]>
>> On Fri, May 6, 2016 at 4:41 AM, Junio C Hamano <[hidden email]> wrote:
>>> "Philip Oakley" <[hidden email]> 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 <[hidden email]>
Subject: [PATCH] -Wuninitialized: remove a gcc specific workaround

Signed-off-by: Ramsay Jones <[hidden email]>
---
 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 [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Philip Oakley
From: "Ramsay Jones" <[hidden email]>

> On 06/05/16 14:15, Philip Oakley wrote:
>> From: "Duy Nguyen" <[hidden email]>
>>> On Fri, May 6, 2016 at 4:41 AM, Junio C Hamano <[hidden email]>
>>> wrote:
>>>> "Philip Oakley" <[hidden email]> 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 <[hidden email]>
> Subject: [PATCH] -Wuninitialized: remove a gcc specific workaround
>
> Signed-off-by: Ramsay Jones <[hidden email]>
> ---
> 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 [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
In reply to this post by Ramsay Jones-2
Ramsay Jones <[hidden email]> 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?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Marc Branchaud <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Stefan Beller-4
On Fri, May 6, 2016 at 12:57 PM, Junio C Hamano <[hidden email]> wrote:

> Marc Branchaud <[hidden email]> 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 [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Ramsay Jones-2
In reply to this post by Junio C Hamano


On 06/05/16 19:54, Junio C Hamano wrote:

> Ramsay Jones <[hidden email]> 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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Marc Branchaud
In reply to this post by Junio C Hamano
On 2016-05-06 03:57 PM, Junio C Hamano wrote:

> Marc Branchaud <[hidden email]> 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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Ramsay Jones-2
In reply to this post by Ramsay Jones-2


On 06/05/16 21:21, Ramsay Jones wrote:
> On 06/05/16 19:54, Junio C Hamano wrote:
>> Ramsay Jones <[hidden email]> 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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Philip Oakley
In reply to this post by Stefan Beller-4
From: "Stefan Beller" <[hidden email]>

> On Fri, May 6, 2016 at 12:57 PM, Junio C Hamano <[hidden email]> wrote:
>> Marc Branchaud <[hidden email]> 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Randall S. Becker
On May 9, 2016 3:40 PM Philip Oakley wrote:

> From: "Stefan Beller" <[hidden email]>
> > On Fri, May 6, 2016 at 12:57 PM, Junio C Hamano <[hidden email]>
> wrote:
> >> Marc Branchaud <[hidden email]> 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.



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html