{"thread":{"id":"39532","subject":"[PATCH] utf8: NO_ICONV: silence uninitialized variable warning","startedAt":"2015-06-05T06:42:16Z","lastAt":"2015-06-05T22:19:04Z","messageCount":3,"participants":["Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"262994","messageId":"1433486536-23447-1-git-send-email-sunshine@sunshineco.com","threadId":"39532","inReplyTo":null,"subject":"[PATCH] utf8: NO_ICONV: silence uninitialized variable warning","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-05T06:42:16Z","receivedAt":"2015-06-05T06:42:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The last argument of reencode_string_len() is an 'int *' which is\nassigned the length of the converted string. When NO_ICONV is defined,\nhowever, reencode_string_len() is stubbed out by the macro:\n\n    #define reencode_string_len(a,b,c,d,e) NULL\n\nwhich never assigns a value to the final argument. When called like\nthis:\n\n    int n;\n    char *s = reencode_string_len(..., &n);\n    if (s)\n        do_something(s, n);\n\nsome compilers complain that 'n' is used uninitialized within the\nconditional.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n utf8.h | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/utf8.h b/utf8.h\nindex e7b2aa4..5a9e94b 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -31,7 +31,9 @@ char *reencode_string_len(const char *in, int insz,\n \t\t\t  const char *in_encoding,\n \t\t\t  int *outsz);\n #else\n-#define reencode_string_len(a,b,c,d,e) NULL\n+static inline char *reencode_string_len(const char *a, int b,\n+\t\t\t\t\tconst char *c, const char *d, int *e)\n+{ if (e) *e = 0; return NULL; }\n #endif\n \n static inline char *reencode_string(const char *in,\n-- \n2.4.2.613.g328fd50\n"},{"id":"262999","messageId":"20150605092309.GA11855@peff.net","threadId":"39532","inReplyTo":"1433486536-23447-1-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH] utf8: NO_ICONV: silence uninitialized variable warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-05T09:23:09Z","receivedAt":"2015-06-05T09:23:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 05, 2015 at 02:42:16AM -0400, Eric Sunshine wrote:\n\n> The last argument of reencode_string_len() is an 'int *' which is\n> assigned the length of the converted string. When NO_ICONV is defined,\n> however, reencode_string_len() is stubbed out by the macro:\n> \n>     #define reencode_string_len(a,b,c,d,e) NULL\n> \n> which never assigns a value to the final argument. When called like\n> this:\n> \n>     int n;\n>     char *s = reencode_string_len(..., &n);\n>     if (s)\n>         do_something(s, n);\n> \n> some compilers complain that 'n' is used uninitialized within the\n> conditional.\n\nHmm, this sounded familiar to me. And indeed:\n\n  http://article.gmane.org/gmane.comp.version-control.git/264095\n\nbut I guess we never pushed that topic forward.\n\n> -#define reencode_string_len(a,b,c,d,e) NULL\n> +static inline char *reencode_string_len(const char *a, int b,\n> +\t\t\t\t\tconst char *c, const char *d, int *e)\n> +{ if (e) *e = 0; return NULL; }\n>  #endif\n\nIn the non-NO_ICONV code path we do not set \"e\" when returning NULL, so\nthis actually behaves differently than the real function. I'm not\npuzzled that the compiler behaves differently (after all, it must assume\nthat the out-parameter is filled in when it is passed to an extern\nfunction, but with the noop here it can easily see that it is not).\n\nBut I am puzzled that the compiler, when given the full code, does not\nrealize that \"s\" in your example is always NULL, and the conditional is\ndead code.\n\nIn the patch linked above, I just initialized the variables in the\ncallers. Your patch here just unconditionally sets the outsz parameter.\nThat is certainly fine when NO_ICONV is set (and nicer, because it\ncontains the fix to one spot), but I wonder if it means we are papering\nover a problem in the normal code path. I.e., now that we give the\ncompiler the extra information about the implementation of\nreencode_string_len, it knows to complain.\n\nBut I just cannot see any way for it to be a problem. The simple code\nexample you gave above is quite accurate.\n\nSo I think your patch is the best option, but it might be good to give\none more look at the callers to be sure we are not missing something.\n\n-Peff\n"},{"id":"263074","messageId":"xmqqeglpre7b.fsf@gitster.dls.corp.google.com","threadId":"39532","inReplyTo":"20150605092309.GA11855@peff.net","subject":"Re: [PATCH] utf8: NO_ICONV: silence uninitialized variable warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-05T22:19:04Z","receivedAt":"2015-06-05T22:19:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So I think your patch is the best option, but it might be good to give\n> one more look at the callers to be sure we are not missing something.\n\nThe two callers both leave the outsz uninitialized and to a human it\nis obvious that uninitialized outsz is never used when the function\nor macro returns NULL.  It is somewhat sad that we need a useless\nassignment to *e to help less than intelligent compilers, but it is\npossible that a clever compiler may be able to optimize the useless\nassignment away so it might even out in the end ;-)\n\nWill queue.\n\nThanks.\n"}]}