* /* compiler workaround */ - what was the issue? [not found] <AA5B2B1715BAF7438221293187A417A7BDE9D11D@desmdswms002.des.grplnk.net> @ 2016-05-05 20:48 ` Philip Oakley 2016-05-05 21:41 ` Junio C Hamano 0 siblings, 1 reply; 15+ messages in thread From: Philip Oakley @ 2016-05-05 20:48 UTC (permalink / raw) To: Git List; +Cc: Duy 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: /* compiler workaround */ - what was the issue? 2016-05-05 20:48 ` /* compiler workaround */ - what was the issue? Philip Oakley @ 2016-05-05 21:41 ` Junio C Hamano 2016-05-06 10:17 ` Duy Nguyen 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2016-05-05 21:41 UTC (permalink / raw) To: Philip Oakley; +Cc: Git List, Duy "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). ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: /* compiler workaround */ - what was the issue? 2016-05-05 21:41 ` Junio C Hamano @ 2016-05-06 10:17 ` Duy Nguyen 2016-05-06 13:15 ` Philip Oakley 0 siblings, 1 reply; 15+ messages in thread From: Duy Nguyen @ 2016-05-06 10:17 UTC (permalink / raw) To: Junio C Hamano; +Cc: Philip Oakley, Git List 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: /* compiler workaround */ - what was the issue? 2016-05-06 10:17 ` Duy Nguyen @ 2016-05-06 13:15 ` Philip Oakley 2016-05-06 18:05 ` Ramsay Jones 0 siblings, 1 reply; 15+ messages in thread From: Philip Oakley @ 2016-05-06 13:15 UTC (permalink / raw) To: Duy Nguyen, Junio C Hamano; +Cc: Git List 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: /* compiler workaround */ - what was the issue? 2016-05-06 13:15 ` Philip Oakley @ 2016-05-06 18:05 ` Ramsay Jones 2016-05-06 18:33 ` Philip Oakley 2016-05-06 18:54 ` Junio C Hamano 0 siblings, 2 replies; 15+ messages in thread From: Ramsay Jones @ 2016-05-06 18:05 UTC (permalink / raw) To: Philip Oakley, Duy Nguyen, Junio C Hamano; +Cc: Git List 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: /* compiler workaround */ - what was the issue? 2016-05-06 18:05 ` Ramsay Jones @ 2016-05-06 18:33 ` Philip Oakley 2016-05-06 18:54 ` Junio C Hamano 1 sibling, 0 replies; 15+ messages in thread From: Philip Oakley @ 2016-05-06 18:33 UTC (permalink / raw) To: Duy Nguyen, Junio C Hamano, Ramsay Jones; +Cc: Git List 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: /* compiler workaround */ - what was the issue? 2016-05-06 18:05 ` Ramsay Jones 2016-05-06 18:33 ` Philip Oakley @ 2016-05-06 18:54 ` Junio C Hamano 2016-05-06 19:30 ` Marc Branchaud 2016-05-06 20:21 ` Ramsay Jones 1 sibling, 2 replies; 15+ messages in thread From: Junio C Hamano @ 2016-05-06 18:54 UTC (permalink / raw) To: Ramsay Jones; +Cc: Philip Oakley, Duy Nguyen, Git List 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? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: /* compiler workaround */ - what was the issue? 2016-05-06 18:54 ` Junio C Hamano @ 2016-05-06 19:30 ` Marc Branchaud 2016-05-06 19:57 ` Junio C Hamano 2016-05-06 20:21 ` Ramsay Jones 1 sibling, 1 reply; 15+ messages in thread From: Marc Branchaud @ 2016-05-06 19:30 UTC (permalink / raw) To: Junio C Hamano, Ramsay Jones; +Cc: Philip Oakley, Duy Nguyen, Git List 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: /* compiler workaround */ - what was the issue? 2016-05-06 19:30 ` Marc Branchaud @ 2016-05-06 19:57 ` Junio C Hamano 2016-05-06 20:01 ` Stefan Beller 2016-05-06 20:28 ` Marc Branchaud 0 siblings, 2 replies; 15+ messages in thread From: Junio C Hamano @ 2016-05-06 19:57 UTC (permalink / raw) To: Marc Branchaud; +Cc: Ramsay Jones, Philip Oakley, Duy Nguyen, Git List 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: /* compiler workaround */ - what was the issue? 2016-05-06 19:57 ` Junio C Hamano @ 2016-05-06 20:01 ` Stefan Beller 2016-05-09 19:40 ` Philip Oakley 2016-05-06 20:28 ` Marc Branchaud 1 sibling, 1 reply; 15+ messages in thread From: Stefan Beller @ 2016-05-06 20:01 UTC (permalink / raw) To: Junio C Hamano Cc: Marc Branchaud, Ramsay Jones, Philip Oakley, Duy Nguyen, Git List 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: /* compiler workaround */ - what was the issue? 2016-05-06 20:01 ` Stefan Beller @ 2016-05-09 19:40 ` Philip Oakley 2016-05-09 19:49 ` Randall S. Becker 0 siblings, 1 reply; 15+ messages in thread From: Philip Oakley @ 2016-05-09 19:40 UTC (permalink / raw) To: Stefan Beller, Junio C Hamano Cc: Marc Branchaud, Ramsay Jones, Duy Nguyen, Git List 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: /* compiler workaround */ - what was the issue? 2016-05-09 19:40 ` Philip Oakley @ 2016-05-09 19:49 ` Randall S. Becker 0 siblings, 0 replies; 15+ messages in thread From: Randall S. Becker @ 2016-05-09 19:49 UTC (permalink / raw) To: 'Philip Oakley', 'Stefan Beller', 'Junio C Hamano' Cc: 'Marc Branchaud', 'Ramsay Jones', 'Duy Nguyen', 'Git List' 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: /* compiler workaround */ - what was the issue? 2016-05-06 19:57 ` Junio C Hamano 2016-05-06 20:01 ` Stefan Beller @ 2016-05-06 20:28 ` Marc Branchaud 1 sibling, 0 replies; 15+ messages in thread From: Marc Branchaud @ 2016-05-06 20:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ramsay Jones, Philip Oakley, Duy Nguyen, Git List 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: /* compiler workaround */ - what was the issue? 2016-05-06 18:54 ` Junio C Hamano 2016-05-06 19:30 ` Marc Branchaud @ 2016-05-06 20:21 ` Ramsay Jones 2016-05-06 21:17 ` Ramsay Jones 1 sibling, 1 reply; 15+ messages in thread From: Ramsay Jones @ 2016-05-06 20:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: Philip Oakley, Duy Nguyen, Git List 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: /* compiler workaround */ - what was the issue? 2016-05-06 20:21 ` Ramsay Jones @ 2016-05-06 21:17 ` Ramsay Jones 0 siblings, 0 replies; 15+ messages in thread From: Ramsay Jones @ 2016-05-06 21:17 UTC (permalink / raw) To: Junio C Hamano; +Cc: Philip Oakley, Duy Nguyen, Git List 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-05-09 19:50 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <AA5B2B1715BAF7438221293187A417A7BDE9D11D@desmdswms002.des.grplnk.net>
2016-05-05 20:48 ` /* compiler workaround */ - what was the issue? Philip Oakley
2016-05-05 21:41 ` Junio C Hamano
2016-05-06 10:17 ` Duy Nguyen
2016-05-06 13:15 ` Philip Oakley
2016-05-06 18:05 ` Ramsay Jones
2016-05-06 18:33 ` Philip Oakley
2016-05-06 18:54 ` Junio C Hamano
2016-05-06 19:30 ` Marc Branchaud
2016-05-06 19:57 ` Junio C Hamano
2016-05-06 20:01 ` Stefan Beller
2016-05-09 19:40 ` Philip Oakley
2016-05-09 19:49 ` Randall S. Becker
2016-05-06 20:28 ` Marc Branchaud
2016-05-06 20:21 ` Ramsay Jones
2016-05-06 21:17 ` Ramsay Jones
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).