{"thread":{"id":"33289","subject":"[PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning","startedAt":"2013-03-26T19:20:11Z","lastAt":"2013-03-28T19:36:38Z","messageCount":6,"participants":["Ramsay Jones","Jeff King","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"212316","messageId":"5151F4EB.80909@ramsay1.demon.co.uk","threadId":"33289","inReplyTo":null,"subject":"[PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-03-26T19:20:11Z","receivedAt":"2013-03-26T19:20:11Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nAfter commit cbfd5e1c (\"drop some obsolete \"x = x\" compiler warning\nhacks\", 21-03-2013) removed a gcc specific hack, older versions of\ngcc now issue an \"'contents' might be used uninitialized\" warning.\nIn order to suppress the warning, we simply initialize the variable\nto NULL in it's declaration.\n\nSigned-off-by: Ramsay Jones \n---\n\nAn alternative solution may look like this (note: *untested*):\n\n diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n index ad29000..e50b20f 100644\n --- a/builtin/cat-file.c\n +++ b/builtin/cat-file.c\n @@ -193,7 +193,6 @@ static int batch_one_object(const char *obj_name, int print_contents)\n \tunsigned char sha1[20];\n \tenum object_type type = 0;\n \tunsigned long size;\n -\tvoid *contents;\n \n \tif (!obj_name)\n \t return 1;\n @@ -204,16 +203,11 @@ static int batch_one_object(const char *obj_name, int print_contents)\n \t\treturn 0;\n \t}\n \n -\tif (print_contents == BATCH)\n -\t\tcontents = read_sha1_file(sha1, &type, &size);\n -\telse\n -\t\ttype = sha1_object_info(sha1, &size);\n +\ttype = sha1_object_info(sha1, &size);\n \n \tif (type <= 0) {\n \t\tprintf(\"%s missing\\n\", obj_name);\n \t\tfflush(stdout);\n -\t\tif (print_contents == BATCH)\n -\t\t\tfree(contents);\n \t\treturn 0;\n \t}\n \n @@ -221,6 +215,7 @@ static int batch_one_object(const char *obj_name, int print_contents)\n \tfflush(stdout);\n \n \tif (print_contents == BATCH) {\n +\t\tvoid *contents = read_sha1_file(sha1, &type, &size);\n \t\twrite_or_die(1, contents, size);\n \t\tprintf(\"\\n\");\n \t\tfflush(stdout);\n -- \n\nHowever, this would add an additional call to sha1_object_info() to\nthe \"--batch\" code path, with potential performance consequences\n(again untested). Also, if you are paranoid, I guess you should\ncheck that the (type,size) returned by sha1_object_info() was the\nsame as that returned by read_sha1_file(). ;-)\n\nATB,\nRamsay Jones\n\n builtin/cat-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex ad29000..40f87b4 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -193,7 +193,7 @@ static int batch_one_object(const char *obj_name, int print_contents)\n \tunsigned char sha1[20];\n \tenum object_type type = 0;\n \tunsigned long size;\n-\tvoid *contents;\n+\tvoid *contents = NULL;\n \n \tif (!obj_name)\n \t return 1;\n-- \n1.8.2\n"},{"id":"212320","messageId":"20130326193539.GB28244@sigill.intra.peff.net","threadId":"33289","inReplyTo":"5151F4EB.80909@ramsay1.demon.co.uk","subject":"Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-26T19:35:39Z","receivedAt":"2013-03-26T19:35:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 07:20:11PM +0000, Ramsay Jones wrote:\n\n> After commit cbfd5e1c (\"drop some obsolete \"x = x\" compiler warning\n> hacks\", 21-03-2013) removed a gcc specific hack, older versions of\n> gcc now issue an \"'contents' might be used uninitialized\" warning.\n> In order to suppress the warning, we simply initialize the variable\n> to NULL in it's declaration.\n\nI'm OK with this, if it's the direction we want to go. But I thought the\ndiscussion kind of ended as \"we do not care about these warnings on\nancient versions of gcc; those people should use -Wno-error=uninitialized\".\n\nWhat version of gcc are you using? If it is the most recent thing\nreasonably available on msysgit, then I am more sympathetic. But if it's\njust an antique version of gcc, I am less so.\n\n> An alternative solution may look like this (note: *untested*):\n> [...]\n> However, this would add an additional call to sha1_object_info()\n\nYeah, I don't think that is worth it.\n\n> the \"--batch\" code path, with potential performance consequences\n> (again untested). Also, if you are paranoid, I guess you should\n> check that the (type,size) returned by sha1_object_info() was the\n> same as that returned by read_sha1_file(). ;-)\n\nI note that we do not actually check that contents != NULL after calling\nread_sha1_file, either (nor that sha1_object_info does not return an\nerror). I suspect cat-file could segfault under the right conditions.\n\n-Peff\n"},{"id":"212321","messageId":"20130326193805.GA28880@sigill.intra.peff.net","threadId":"33289","inReplyTo":"20130326193539.GB28244@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-26T19:38:05Z","receivedAt":"2013-03-26T19:38:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2013 at 03:35:39PM -0400, Jeff King wrote:\n\n> I note that we do not actually check that contents != NULL after calling\n> read_sha1_file, either (nor that sha1_object_info does not return an\n> error). I suspect cat-file could segfault under the right conditions.\n\nOh nevermind, we do. We just do it by checking that \"type\" was filled in\ncorrectly, which can combine the check for both read_sha1_file and\nsha1_object_info. Sorry for the noise.\n\n-Peff\n"},{"id":"212564","messageId":"5154908B.1040801@ramsay1.demon.co.uk","threadId":"33289","inReplyTo":"20130326193539.GB28244@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-03-28T18:48:43Z","receivedAt":"2013-03-28T18:48:43Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jeff King wrote:\n> On Tue, Mar 26, 2013 at 07:20:11PM +0000, Ramsay Jones wrote:\n> \n>> After commit cbfd5e1c (\"drop some obsolete \"x = x\" compiler warning\n>> hacks\", 21-03-2013) removed a gcc specific hack, older versions of\n>> gcc now issue an \"'contents' might be used uninitialized\" warning.\n>> In order to suppress the warning, we simply initialize the variable\n>> to NULL in it's declaration.\n> \n> I'm OK with this, if it's the direction we want to go. But I thought the\n> discussion kind of ended as \"we do not care about these warnings on\n> ancient versions of gcc; those people should use -Wno-error=uninitialized\".\n\nHmm, I don't recall any agreement or conclusions being reached.\nI guess I missed that!\n\n> What version of gcc are you using? If it is the most recent thing\n> reasonably available on msysgit, then I am more sympathetic. But if it's\n> just an antique version of gcc, I am less so.\n\n(see previous email for compiler versions).\n\nI suppose it depends on what you consider antique. [I recently\ndownloaded the \"first C compiler\" from github. Yes, that is an\nantique compiler! ;-)] I would call some of the compilers I use\n\"a bit mature.\" :-P\n\nHmm, so are you saying that this patch is not acceptable because\nI used a compiler that is no longer supported?\n\nATB,\nRamsay Jones\n"},{"id":"212567","messageId":"20130328190226.GB17178@sigill.intra.peff.net","threadId":"33289","inReplyTo":"5154908B.1040801@ramsay1.demon.co.uk","subject":"Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-28T19:02:26Z","receivedAt":"2013-03-28T19:02:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 28, 2013 at 06:48:43PM +0000, Ramsay Jones wrote:\n\n> > I'm OK with this, if it's the direction we want to go. But I thought the\n> > discussion kind of ended as \"we do not care about these warnings on\n> > ancient versions of gcc; those people should use -Wno-error=uninitialized\".\n> \n> Hmm, I don't recall any agreement or conclusions being reached.\n> I guess I missed that!\n\nI think Jonathan said that and nobody disagreed, and I took it as a\nconclusion.\n\n> Hmm, so are you saying that this patch is not acceptable because\n> I used a compiler that is no longer supported?\n\nNo, I just think we should come to a decision on how unreadable to make\nthe code in order to suppress incorrect warnings on old compilers. I can\nsee the point in either of the following arguments:\n\n 1. These compilers are old, and we do not need to cater to them in the\n code because people can just _not_ set -Werror=uninitialized (or\n its equivalent). It is still worth catering to bugs in modern\n compilers that most devs use, because being able to set -Werror is\n helpful.\n\n 2. The code is not made significantly less readable, especially if you\n put in a comment, so why not help these compilers.\n\nWhen we can make the code more readable _and_ help the compiler, I think\nit is a no-brainer. I am on the fence otherwise and don't care that\nmuch. I just think we should apply the rule consistently.\n\n-Peff\n"},{"id":"212572","messageId":"20130328193638.GT28148@google.com","threadId":"33289","inReplyTo":"20130328190226.GB17178@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] cat-file: Fix an gcc -Wuninitialized warning","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-28T19:36:38Z","receivedAt":"2013-03-28T19:36:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> When we can make the code more readable _and_ help the compiler, I think\n> it is a no-brainer.\n\nYep. :)\n"}]}