{"thread":{"id":"42229","subject":"/* compiler workaround */ - what was the issue?","startedAt":"2016-05-05T20:48:40Z","lastAt":"2016-05-09T19:49:53Z","messageCount":15,"participants":["Philip Oakley","Junio C Hamano","Duy Nguyen","Ramsay Jones","Marc Branchaud","Stefan Beller","Randall S. Becker"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"285600","messageId":"17E04501C9474282B87758C7998A1F5B@PhilipOakley","threadId":"42229","inReplyTo":"AA5B2B1715BAF7438221293187A417A7BDE9D11D@desmdswms002.des.grplnk.net","subject":"/* compiler workaround */ - what was the issue?","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2016-05-05T20:48:40Z","receivedAt":"2016-05-05T20:48:40Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Duy,\n\nIn \nhttps://github.com/git-for-windows/git/commit/b3c96fb158f05152336f167076f5d81d23c3a5e5\n(split-index: strip pathname of on-disk replaced entries, 13 Jun 2014) \nNguyễn Thái Ngọc Duy <pclouds@gmail.com>  you have\n\nread-cache.c#L1790 (in that commit, now ~L1873)\n\n    int saved_namelen = saved_namelen; /* compiler workaround */\n\nWhich then becomes an MSVC compile warning C4700: uninitialized local \nvariable.\n\nI'm wondering what was the compiler workaround being referred to? i.e. why \ndoes it need that tweak? There's no mention of the reason in the commit \nmessage.\n\nThere are a few other occurences of this same initialisation issue, so I was \nwondering if it's still neded? Or should I just get MSVC to ignore it, which \nmay hide other issues..\n\n--\nPhilip\n"},{"id":"285607","messageId":"xmqqtwic9o88.fsf@gitster.mtv.corp.google.com","threadId":"42229","inReplyTo":"17E04501C9474282B87758C7998A1F5B@PhilipOakley","subject":"Re: /* compiler workaround */ - what was the issue?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T21:41:43Z","receivedAt":"2016-05-05T21:41:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" <philipoakley@iee.org> writes:\n\n>     int saved_namelen = saved_namelen; /* compiler workaround */\n>\n> Which then becomes an MSVC compile warning C4700: uninitialized local \n> variable.\n>\n> I'm wondering what was the compiler workaround being referred to? i.e. why \n> does it need that tweak? There's no mention of the reason in the commit \n> message.\n\nThat was a fairly well-known workaround for GCC that issues a false\nwarning that variable is used before initialized.  I thought we\nstopped using it several years ago in new code after doing a bulk\nsanitizing (I think the new recommended workaround was to initialise\nsuch a variable to the nil value like '0' for integers).\n"},{"id":"285677","messageId":"CACsJy8CBuU8H8r_f4KsnLkhLtfRv0nDo4hGS31LVn0e1Y_3OAQ@mail.gmail.com","threadId":"42229","inReplyTo":"xmqqtwic9o88.fsf@gitster.mtv.corp.google.com","subject":"Re: /* compiler workaround */ - what was the issue?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-06T10:17:56Z","receivedAt":"2016-05-06T10:17:56Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 6, 2016 at 4:41 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> \"Philip Oakley\" <philipoakley@iee.org> writes:\n>\n>>     int saved_namelen = saved_namelen; /* compiler workaround */\n>>\n>> Which then becomes an MSVC compile warning C4700: uninitialized local\n>> variable.\n>>\n>> I'm wondering what was the compiler workaround being referred to? i.e. why\n>> does it need that tweak? There's no mention of the reason in the commit\n>> message.\n>\n> That was a fairly well-known workaround for GCC that issues a false\n> warning that variable is used before initialized.  I thought we\n> stopped using it several years ago in new code after doing a bulk\n> sanitizing\n\nI guess that's 803a777 (cat-file: Fix an gcc -Wuninitialized warning -\n2013-03-26) and more commits around that time. The split-index commit\nis in 2014. I must have missed the trend.\n\n> (I think the new recommended workaround was to initialise\n> such a variable to the nil value like '0' for integers).\n\nYep. First Jeff removed the \" = xxx\" part from \"xxx = xxx\" then Ramsay\nadded the \" = NULL\" back. So we probably just do \"int saved_namelen =\n0;\" in this case.\n-- \nDuy\n"},{"id":"285698","messageId":"51C902B1F7464CF2B58EB0E495F86BB5@PhilipOakley","threadId":"42229","inReplyTo":"CACsJy8CBuU8H8r_f4KsnLkhLtfRv0nDo4hGS31LVn0e1Y_3OAQ@mail.gmail.com","subject":"Re: /* compiler workaround */ - what was the issue?","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2016-05-06T13:15:38Z","receivedAt":"2016-05-06T13:15:38Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Duy Nguyen\" <pclouds@gmail.com>\n> On Fri, May 6, 2016 at 4:41 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> \"Philip Oakley\" <philipoakley@iee.org> writes:\n>>\n>>>     int saved_namelen = saved_namelen; /* compiler workaround */\n>>>\n>>> Which then becomes an MSVC compile warning C4700: uninitialized local\n>>> variable.\n>>>\n>>> I'm wondering what was the compiler workaround being referred to? i.e. \n>>> why\n>>> does it need that tweak? There's no mention of the reason in the commit\n>>> message.\n>>\n>> That was a fairly well-known workaround for GCC that issues a false\n>> warning that variable is used before initialized.  I thought we\n>> stopped using it several years ago in new code after doing a bulk\n>> sanitizing\n>\n> I guess that's 803a777 (cat-file: Fix an gcc -Wuninitialized warning -\n> 2013-03-26) and more commits around that time. The split-index commit\n> is in 2014. I must have missed the trend.\n>\n>> (I think the new recommended workaround was to initialise\n>> such a variable to the nil value like '0' for integers).\n>\n> Yep. First Jeff removed the \" = xxx\" part from \"xxx = xxx\" then Ramsay\n> added the \" = NULL\" back. So we probably just do \"int saved_namelen =\n> 0;\" in this case.\n> -- \nThanks,\n\nI'll try and work up a patch - probably next week as I'm away for the \nweekend.\n\n--\nPhilip \n"},{"id":"285764","messageId":"572CDCFF.9050607@ramsayjones.plus.com","threadId":"42229","inReplyTo":"51C902B1F7464CF2B58EB0E495F86BB5@PhilipOakley","subject":"Re: /* compiler workaround */ - what was the issue?","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-05-06T18:05:51Z","receivedAt":"2016-05-06T18:05:51Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 06/05/16 14:15, Philip Oakley wrote:\n> From: \"Duy Nguyen\" <pclouds@gmail.com>\n>> On Fri, May 6, 2016 at 4:41 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> \"Philip Oakley\" <philipoakley@iee.org> writes:\n>>>\n>>>>     int saved_namelen = saved_namelen; /* compiler workaround */\n>>>>\n>>>> Which then becomes an MSVC compile warning C4700: uninitialized local\n>>>> variable.\n>>>>\n>>>> I'm wondering what was the compiler workaround being referred to? i.e. why\n>>>> does it need that tweak? There's no mention of the reason in the commit\n>>>> message.\n>>>\n>>> That was a fairly well-known workaround for GCC that issues a false\n>>> warning that variable is used before initialized.  I thought we\n>>> stopped using it several years ago in new code after doing a bulk\n>>> sanitizing\n>>\n>> I guess that's 803a777 (cat-file: Fix an gcc -Wuninitialized warning -\n>> 2013-03-26) and more commits around that time. The split-index commit\n>> is in 2014. I must have missed the trend.\n>>\n>>> (I think the new recommended workaround was to initialise\n>>> such a variable to the nil value like '0' for integers).\n>>\n>> Yep. First Jeff removed the \" = xxx\" part from \"xxx = xxx\" then Ramsay\n>> added the \" = NULL\" back. So we probably just do \"int saved_namelen =\n>> 0;\" in this case.\n>> -- \n> Thanks,\n> \n> I'll try and work up a patch - probably next week as I'm away for the weekend.\n\nYeah, I don't remember why these were left over from the previous\nattempt to clean these up (maybe they conflicted with in-flight\ntopics?), but I have had a patch hanging around ... :-D\n\nThe patch below applies to master (I haven't checked for any more\nadditions).\n\nATB,\nRamsay Jones\n\n-- >8 --\nFrom: Ramsay Jones <ramsay@ramsayjones.plus.com>\nSubject: [PATCH] -Wuninitialized: remove a gcc specific workaround\n\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n builtin/rev-list.c | 2 +-\n fast-import.c      | 4 ++--\n merge-recursive.c  | 2 +-\n read-cache.c       | 2 +-\n 4 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 275da0d..deae1f3 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -377,7 +377,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\tmark_edges_uninteresting(&revs, show_edge);\n \n \tif (bisect_list) {\n-\t\tint reaches = reaches, all = all;\n+\t\tint reaches = 0, all = 0;\n \n \t\trevs.commits = find_bisection(revs.commits, &reaches, &all,\n \t\t\t\t\t      bisect_find_all);\ndiff --git a/fast-import.c b/fast-import.c\nindex 9fc7093..ca66d80 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2935,7 +2935,7 @@ static void cat_blob(struct object_entry *oe, unsigned char sha1[20])\n \n static void parse_get_mark(const char *p)\n {\n-\tstruct object_entry *oe = oe;\n+\tstruct object_entry *oe = NULL;\n \tchar output[42];\n \n \t/* get-mark SP <object> LF */\n@@ -2952,7 +2952,7 @@ static void parse_get_mark(const char *p)\n \n static void parse_cat_blob(const char *p)\n {\n-\tstruct object_entry *oe = oe;\n+\tstruct object_entry *oe = NULL;\n \tunsigned char sha1[20];\n \n \t/* cat-blob SP <object> LF */\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 06d31ed..9cecc24 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1897,7 +1897,7 @@ int merge_recursive(struct merge_options *o,\n {\n \tstruct commit_list *iter;\n \tstruct commit *merged_common_ancestors;\n-\tstruct tree *mrtree = mrtree;\n+\tstruct tree *mrtree = NULL;\n \tint clean;\n \n \tif (show(o, 4)) {\ndiff --git a/read-cache.c b/read-cache.c\nindex d9fb78b..978d6b6 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1870,7 +1870,7 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n {\n \tint size;\n \tstruct ondisk_cache_entry *ondisk;\n-\tint saved_namelen = saved_namelen; /* compiler workaround */\n+\tint saved_namelen = 0;\n \tchar *name;\n \tint result;\n \n-- \n2.8.0\n"},{"id":"285767","messageId":"0727E275927743DF94CFEC8615D7E90B@PhilipOakley","threadId":"42229","inReplyTo":"572CDCFF.9050607@ramsayjones.plus.com","subject":"Re: /* compiler workaround */ - what was the issue?","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2016-05-06T18:33:13Z","receivedAt":"2016-05-06T18:33:13Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Ramsay Jones\" <ramsay@ramsayjones.plus.com>\n> On 06/05/16 14:15, Philip Oakley wrote:\n>> From: \"Duy Nguyen\" <pclouds@gmail.com>\n>>> On Fri, May 6, 2016 at 4:41 AM, Junio C Hamano <gitster@pobox.com> \n>>> wrote:\n>>>> \"Philip Oakley\" <philipoakley@iee.org> writes:\n>>>>\n>>>>>     int saved_namelen = saved_namelen; /* compiler workaround */\n>>>>>\n>>>>> Which then becomes an MSVC compile warning C4700: uninitialized local\n>>>>> variable.\n>>>>>\n>>>>> I'm wondering what was the compiler workaround being referred to? i.e. \n>>>>> why\n>>>>> does it need that tweak? There's no mention of the reason in the \n>>>>> commit\n>>>>> message.\n>>>>\n>>>> That was a fairly well-known workaround for GCC that issues a false\n>>>> warning that variable is used before initialized.  I thought we\n>>>> stopped using it several years ago in new code after doing a bulk\n>>>> sanitizing\n>>>\n>>> I guess that's 803a777 (cat-file: Fix an gcc -Wuninitialized warning -\n>>> 2013-03-26) and more commits around that time. The split-index commit\n>>> is in 2014. I must have missed the trend.\n>>>\n>>>> (I think the new recommended workaround was to initialise\n>>>> such a variable to the nil value like '0' for integers).\n>>>\n>>> Yep. First Jeff removed the \" = xxx\" part from \"xxx = xxx\" then Ramsay\n>>> added the \" = NULL\" back. So we probably just do \"int saved_namelen =\n>>> 0;\" in this case.\n>>> -- \n>> Thanks,\n>>\n>> I'll try and work up a patch - probably next week as I'm away for the \n>> weekend.\n>\n> Yeah, I don't remember why these were left over from the previous\n> attempt to clean these up (maybe they conflicted with in-flight\n> topics?), but I have had a patch hanging around ... :-D\n>\n> The patch below applies to master (I haven't checked for any more\n> additions).\n\nLooks good to me. Catches those I've seen.\n\nHave a good weekend all.\n\n>\n> ATB,\n> Ramsay Jones\n>\n> -- >8 --\n> From: Ramsay Jones <ramsay@ramsayjones.plus.com>\n> Subject: [PATCH] -Wuninitialized: remove a gcc specific workaround\n>\n> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n> ---\n> builtin/rev-list.c | 2 +-\n> fast-import.c      | 4 ++--\n> merge-recursive.c  | 2 +-\n> read-cache.c       | 2 +-\n> 4 files changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index 275da0d..deae1f3 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -377,7 +377,7 @@ int cmd_rev_list(int argc, const char **argv, const \n> char *prefix)\n>  mark_edges_uninteresting(&revs, show_edge);\n>\n>  if (bisect_list) {\n> - int reaches = reaches, all = all;\n> + int reaches = 0, all = 0;\n>\n>  revs.commits = find_bisection(revs.commits, &reaches, &all,\n>        bisect_find_all);\n> diff --git a/fast-import.c b/fast-import.c\n> index 9fc7093..ca66d80 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -2935,7 +2935,7 @@ static void cat_blob(struct object_entry *oe, \n> unsigned char sha1[20])\n>\n> static void parse_get_mark(const char *p)\n> {\n> - struct object_entry *oe = oe;\n> + struct object_entry *oe = NULL;\n>  char output[42];\n>\n>  /* get-mark SP <object> LF */\n> @@ -2952,7 +2952,7 @@ static void parse_get_mark(const char *p)\n>\n> static void parse_cat_blob(const char *p)\n> {\n> - struct object_entry *oe = oe;\n> + struct object_entry *oe = NULL;\n>  unsigned char sha1[20];\n>\n>  /* cat-blob SP <object> LF */\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 06d31ed..9cecc24 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1897,7 +1897,7 @@ int merge_recursive(struct merge_options *o,\n> {\n>  struct commit_list *iter;\n>  struct commit *merged_common_ancestors;\n> - struct tree *mrtree = mrtree;\n> + struct tree *mrtree = NULL;\n>  int clean;\n>\n>  if (show(o, 4)) {\n> diff --git a/read-cache.c b/read-cache.c\n> index d9fb78b..978d6b6 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1870,7 +1870,7 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, \n> struct cache_entry *ce,\n> {\n>  int size;\n>  struct ondisk_cache_entry *ondisk;\n> - int saved_namelen = saved_namelen; /* compiler workaround */\n> + int saved_namelen = 0;\n>  char *name;\n>  int result;\n>\n> -- \n> 2.8.0\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"285769","messageId":"xmqq60ur3tlu.fsf@gitster.mtv.corp.google.com","threadId":"42229","inReplyTo":"572CDCFF.9050607@ramsayjones.plus.com","subject":"Re: /* compiler workaround */ - what was the issue?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T18:54:21Z","receivedAt":"2016-05-06T18:54:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> The patch below applies to master (I haven't checked for any more\n> additions).\n>\n>  \tif (bisect_list) {\n> -\t\tint reaches = reaches, all = all;\n> +\t\tint reaches = 0, all = 0;\n\nOne thing that is somewhat sad is that this makes future readers\nwonder if these values '0' are sensible initial values.\n\nHaving to wonder \"is it sensible to initialize this variable to 0?\nShouldn't it be initialized to INT_MAX instead?\" is wasting their\ntime exactly because we _know_ these are not even \"initial values\".\nWe know these do not have to be initialized, because some more\nappropriate values will get assigned to them before they are used,\nand have these only because some compilers get it wrong.\n\nThe original \"reaches = reaches\" had the documentation value (at\nleast for those who knew the convention) to save the readers from\nwasting their time that way.  Now these 0 are indistinguishable from\nthe other initializations that require to be zero.\n\n> diff --git a/read-cache.c b/read-cache.c\n> index d9fb78b..978d6b6 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1870,7 +1870,7 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n>  {\n>  \tint size;\n>  \tstruct ondisk_cache_entry *ondisk;\n> -\tint saved_namelen = saved_namelen; /* compiler workaround */\n> +\tint saved_namelen = 0;\n\nI wonder if can we come up with a short and sweet notation to remind\nfuthre readers that this \"initialization\" is not initializing but\nmerely squelching warnings from stupid compilers, and agree to use\nit consistently?\n"},{"id":"285773","messageId":"572CF0D5.6010305@xiplink.com","threadId":"42229","inReplyTo":"xmqq60ur3tlu.fsf@gitster.mtv.corp.google.com","subject":"Re: /* compiler workaround */ - what was the issue?","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2016-05-06T19:30:29Z","receivedAt":"2016-05-06T19:30:29Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 2016-05-06 02:54 PM, Junio C Hamano wrote:\n>\n> I wonder if can we come up with a short and sweet notation to remind\n> futhre readers that this \"initialization\" is not initializing but\n> merely squelching warnings from stupid compilers, and agree to use\n> it consistently?\n\nPerhaps\n\n\t#define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0\n\nor, for short-and-sweet\n\n\t#define CUWI 0\n\n?\n\n:)\n\n\t\tM.\n"},{"id":"285776","messageId":"xmqqinyr2c3d.fsf@gitster.mtv.corp.google.com","threadId":"42229","inReplyTo":"572CF0D5.6010305@xiplink.com","subject":"Re: /* compiler workaround */ - what was the issue?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T19:57:58Z","receivedAt":"2016-05-06T19:57:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marc Branchaud <marcnarc@xiplink.com> writes:\n\n> On 2016-05-06 02:54 PM, Junio C Hamano wrote:\n>>\n>> I wonder if can we come up with a short and sweet notation to remind\n>> futhre readers that this \"initialization\" is not initializing but\n>> merely squelching warnings from stupid compilers, and agree to use\n>> it consistently?\n>\n> Perhaps\n>\n> \t#define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0\n>\n> or, for short-and-sweet\n>\n> \t#define CUWI 0\n>\n> ?\n>\n> :)\n\nI get that smiley.\n\nI was hinting to keep the /* compiler workaround */ comment, but\nin a bit shorter form.\n"},{"id":"285777","messageId":"CAGZ79kby0Z1FMUT-w8h=YfRxsmyXaiE2pA_VoJ0g9wn0Mzk2Wg@mail.gmail.com","threadId":"42229","inReplyTo":"xmqqinyr2c3d.fsf@gitster.mtv.corp.google.com","subject":"Re: /* compiler workaround */ - what was the issue?","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-06T20:01:34Z","receivedAt":"2016-05-06T20:01:34Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 6, 2016 at 12:57 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Marc Branchaud <marcnarc@xiplink.com> writes:\n>\n>> On 2016-05-06 02:54 PM, Junio C Hamano wrote:\n>>>\n>>> I wonder if can we come up with a short and sweet notation to remind\n>>> futhre readers that this \"initialization\" is not initializing but\n>>> merely squelching warnings from stupid compilers, and agree to use\n>>> it consistently?\n>>\n>> Perhaps\n>>\n>>       #define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0\n>>\n>> or, for short-and-sweet\n>>\n\n           /* Here could go a longer explanation than the 4 character\ndefine :) */\n>>       #define CUWI 0\n>>\n>> ?\n>>\n>> :)\n>\n> I get that smiley.\n>\n> I was hinting to keep the /* compiler workaround */ comment, but\n> in a bit shorter form.\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"285780","messageId":"572CFCBC.3020103@ramsayjones.plus.com","threadId":"42229","inReplyTo":"xmqq60ur3tlu.fsf@gitster.mtv.corp.google.com","subject":"Re: /* compiler workaround */ - what was the issue?","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-05-06T20:21:16Z","receivedAt":"2016-05-06T20:21:16Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 06/05/16 19:54, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n> \n>> The patch below applies to master (I haven't checked for any more\n>> additions).\n>>\n>>  \tif (bisect_list) {\n>> -\t\tint reaches = reaches, all = all;\n>> +\t\tint reaches = 0, all = 0;\n> \n> One thing that is somewhat sad is that this makes future readers\n> wonder if these values '0' are sensible initial values.\n> \n> Having to wonder \"is it sensible to initialize this variable to 0?\n> Shouldn't it be initialized to INT_MAX instead?\" is wasting their\n> time exactly because we _know_ these are not even \"initial values\".\n> We know these do not have to be initialized, because some more\n> appropriate values will get assigned to them before they are used,\n> and have these only because some compilers get it wrong.\n> \n> The original \"reaches = reaches\" had the documentation value (at\n> least for those who knew the convention) to save the readers from\n> wasting their time that way.  Now these 0 are indistinguishable from\n> the other initializations that require to be zero.\n\nAh, I think I remember now why I hadn't sent this patch before. ;-)\n[This started off as one patch, was then split into two (int and pointer),\nand then back into one again - presumably because I had by that time\nforgotten why I split it up!]\n\nI have a very vague recollection of you expressing your dislike of\nthese parts of the patch before. I had intended to investigate why\ngcc was incorrectly issuing these warnings - but I couldn't get my\ncurrently installed compiler to complain. That would have meant\nbuilding various gcc versions, so that I could bisect ...\n\nSo, that's why I didn't get around to it ... :-D\n\nI still can't get gcc to complain, e.g. (on top of above):\n\n  $ git diff\n  diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n  index deae1f3..845fcdc 100644\n  --- a/builtin/rev-list.c\n  +++ b/builtin/rev-list.c\n  @@ -377,7 +377,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n                  mark_edges_uninteresting(&revs, show_edge);\n   \n          if (bisect_list) {\n  -               int reaches = 0, all = 0;\n  +               int reaches, all;\n   \n                  revs.commits = find_bisection(revs.commits, &reaches, &all,\n                                                bisect_find_all);\n  $ rm builtin/rev-list.o\n  $ make V=1 CFLAGS='-g -O3 -Wall -Wextra -Wuninitialized -Wno-unused-parameter' builtin/rev-list.o\n  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\n  In file included from ./cache.h:4:0,\n                   from builtin/rev-list.c:1:\n  ./git-compat-util.h: In function ‘xsize_t’:\n  ./git-compat-util.h:838:10: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]\n    if (len > (size_t) len)\n            ^\n  $ \n  \n[Note: gcc (Ubuntu 4.8.4-2ubuntu1~14.04.1) 4.8.4]\n\n> \n>> diff --git a/read-cache.c b/read-cache.c\n>> index d9fb78b..978d6b6 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -1870,7 +1870,7 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,\n>>  {\n>>  \tint size;\n>>  \tstruct ondisk_cache_entry *ondisk;\n>> -\tint saved_namelen = saved_namelen; /* compiler workaround */\n>> +\tint saved_namelen = 0;\n> \n> I wonder if can we come up with a short and sweet notation to remind\n> futhre readers that this \"initialization\" is not initializing but\n> merely squelching warnings from stupid compilers, and agree to use\n> it consistently?\n\nNothing comes to mind.\n\nDo current compilers complain?\n\nATB,\nRamsay Jones\n"},{"id":"285781","messageId":"572CFE7D.1050601@xiplink.com","threadId":"42229","inReplyTo":"xmqqinyr2c3d.fsf@gitster.mtv.corp.google.com","subject":"Re: /* compiler workaround */ - what was the issue?","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2016-05-06T20:28:45Z","receivedAt":"2016-05-06T20:28:45Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 2016-05-06 03:57 PM, Junio C Hamano wrote:\n> Marc Branchaud <marcnarc@xiplink.com> writes:\n>\n>> On 2016-05-06 02:54 PM, Junio C Hamano wrote:\n>>>\n>>> I wonder if can we come up with a short and sweet notation to remind\n>>> futhre readers that this \"initialization\" is not initializing but\n>>> merely squelching warnings from stupid compilers, and agree to use\n>>> it consistently?\n>>\n>> Perhaps\n>>\n>> \t#define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0\n>>\n>> or, for short-and-sweet\n>>\n>> \t#define CUWI 0\n>>\n>> ?\n>>\n>> :)\n>\n> I get that smiley.\n\nOf course, right after I sent that I thought of\n\n#define SPURIOUS_COMPILER_RELATED_UNINITIALIZED_WARNING_INITIALIZER 0\n\nor\n\n#define SCRUWI 0\n\nWhich we'd get to pronounce as \"screwy\".\n\nOK, I'll shut up now.\n\n\t\tM.\n"},{"id":"285784","messageId":"572D09D5.4060600@ramsayjones.plus.com","threadId":"42229","inReplyTo":"572CFCBC.3020103@ramsayjones.plus.com","subject":"Re: /* compiler workaround */ - what was the issue?","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-05-06T21:17:09Z","receivedAt":"2016-05-06T21:17:09Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 06/05/16 21:21, Ramsay Jones wrote:\n> On 06/05/16 19:54, Junio C Hamano wrote:\n>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n>>\n[snip]\n\n> I still can't get gcc to complain, e.g. (on top of above):\n> \n>   $ git diff\n>   diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n>   index deae1f3..845fcdc 100644\n>   --- a/builtin/rev-list.c\n>   +++ b/builtin/rev-list.c\n>   @@ -377,7 +377,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n>                   mark_edges_uninteresting(&revs, show_edge);\n>    \n>           if (bisect_list) {\n>   -               int reaches = 0, all = 0;\n>   +               int reaches, all;\n>    \n>                   revs.commits = find_bisection(revs.commits, &reaches, &all,\n>                                                 bisect_find_all);\n>   $ rm builtin/rev-list.o\n>   $ make V=1 CFLAGS='-g -O3 -Wall -Wextra -Wuninitialized -Wno-unused-parameter' builtin/rev-list.o\n>   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\n>   In file included from ./cache.h:4:0,\n>                    from builtin/rev-list.c:1:\n>   ./git-compat-util.h: In function ‘xsize_t’:\n>   ./git-compat-util.h:838:10: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]\n>     if (len > (size_t) len)\n>             ^\n>   $ \n\nBTW, another patch that I have hanging around ... this time the\npatch below was built on master from a couple of years ago (so,\nI haven't bothered to rebase it, but you should get the idea):\n\nATB,\nRamsay Jones\n\n-- >8 --\nSubject: [PATCH] git-compat-util.h: xsize_t(): avoid signed comparison warnings\n\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n git-compat-util.h | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c07e0c1..3a9cf6c 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -838,9 +838,10 @@ static inline char *xstrdup_or_null(const char *str)\n \n static inline size_t xsize_t(off_t len)\n {\n-\tif (len > (size_t) len)\n+\tsize_t r = (size_t)len;\n+\tif (len != (off_t)r)\n \t\tdie(\"Cannot handle files this big\");\n-\treturn (size_t)len;\n+\treturn r;\n }\n \n __attribute__((format (printf, 3, 4)))\n-- \n2.8.0\n"},{"id":"285964","messageId":"9836AC86AA754C27AFD5D87519DF1402@PhilipOakley","threadId":"42229","inReplyTo":"CAGZ79kby0Z1FMUT-w8h=YfRxsmyXaiE2pA_VoJ0g9wn0Mzk2Wg@mail.gmail.com","subject":"Re: /* compiler workaround */ - what was the issue?","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2016-05-09T19:40:22Z","receivedAt":"2016-05-09T19:40:22Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Stefan Beller\" <sbeller@google.com>\n> On Fri, May 6, 2016 at 12:57 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Marc Branchaud <marcnarc@xiplink.com> writes:\n>>\n>>> On 2016-05-06 02:54 PM, Junio C Hamano wrote:\n>>>>\n>>>> I wonder if can we come up with a short and sweet notation to remind\n>>>> futhre readers that this \"initialization\" is not initializing but\n>>>> merely squelching warnings from stupid compilers, and agree to use\n>>>> it consistently?\n>>>\n>>> Perhaps\n>>>\n>>>       #define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0\n>>>\n>>> or, for short-and-sweet\n>>>\n>\n>           /* Here could go a longer explanation than the 4 character\n> define :) */\n>>>       #define CUWI 0\n>>>\n>>> ?\n>>>\n>>> :)\n>>\n>> I get that smiley.\n>>\n>> I was hinting to keep the /* compiler workaround */ comment, but\n>> in a bit shorter form.\n>> --\n\nFor some background, I found $gmane/169098/focus=169104 which covers some of \nthe issues (the focused msg is one from Junio). Hannes then notes \n($gmane/169121) that the current xx = xx; could be seen as possible \nundefined behaviour - perhaps one of those 'no good solution' problems.\n\nPerhaps a suitable name...\n\n#define SUPPRESS_COMPILER_UNINITIALIZED_WARNING 0\n/* Use when some in-use compiler is unable to determine if the variable is \nused uninitialized, and no good default value is available */\n\nThough, where best to put it?\n\n--\nPhilip \n"},{"id":"285965","messageId":"009801d1aa2b$f89f2530$e9dd6f90$@nexbridge.com","threadId":"42229","inReplyTo":"9836AC86AA754C27AFD5D87519DF1402@PhilipOakley","subject":"RE: /* compiler workaround */ - what was the issue?","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2016-05-09T19:49:53Z","receivedAt":"2016-05-09T19:49:53Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On May 9, 2016 3:40 PM Philip Oakley wrote:\n> From: \"Stefan Beller\" <sbeller@google.com>\n> > On Fri, May 6, 2016 at 12:57 PM, Junio C Hamano <gitster@pobox.com>\n> wrote:\n> >> Marc Branchaud <marcnarc@xiplink.com> writes:\n> >>\n> >>> On 2016-05-06 02:54 PM, Junio C Hamano wrote:\n> >>>>\n> >>>> I wonder if can we come up with a short and sweet notation to\n> >>>> remind futhre readers that this \"initialization\" is not\n> >>>> initializing but merely squelching warnings from stupid compilers,\n> >>>> and agree to use it consistently?\n> >>>\n> >>> Perhaps\n> >>>\n> >>>       #define COMPILER_UNINITIALIZED_WARNING_INITIALIZER 0\n> >>>\n> >>> or, for short-and-sweet\n> >>>\n> >\n> >           /* Here could go a longer explanation than the 4 character\n> > define :) */\n> >>>       #define CUWI 0\n> >>>\n> >>> ?\n> >>>\n> >>> :)\n> >>\n> >> I get that smiley.\n> >>\n> >> I was hinting to keep the /* compiler workaround */ comment, but in a\n> >> bit shorter form.\n> >> --\n> \n> For some background, I found $gmane/169098/focus=169104 which covers\n> some of the issues (the focused msg is one from Junio). Hannes then notes\n> ($gmane/169121) that the current xx = xx; could be seen as possible\n> undefined behaviour - perhaps one of those 'no good solution' problems.\n> \n> Perhaps a suitable name...\n> \n> #define SUPPRESS_COMPILER_UNINITIALIZED_WARNING 0\n> /* Use when some in-use compiler is unable to determine if the variable is\n> used uninitialized, and no good default value is available */\n> \n> Though, where best to put it?\n\nI 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.\n\n-- Brief whoami: NonStop&UNIX developer since approximately UNIX(421664400)/NonStop(211288444200000000)\n-- In my real life, I talk too much.\n"}]}