{"thread":{"id":"21998","subject":"possible code error at run_command.c","startedAt":"2009-12-21T06:46:41Z","lastAt":"2009-12-21T09:13:19Z","messageCount":11,"participants":["Frank Li","Johannes Sixt","Wincent Colaiuta","Erik Faye-Lund"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"130199","messageId":"1976ea660912202246k45732bf2p111bbeb78047693e@mail.gmail.com","threadId":"21998","inReplyTo":null,"subject":"possible code error at run_command.c","fromName":"Frank Li","fromEmail":"lznuaa@gmail.com","sentAt":"2009-12-21T06:46:41Z","receivedAt":"2009-12-21T06:46:41Z","isPatch":false,"sender":{"key":"lznuaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40642?v=4"},"body":"int start_command(struct child_process *cmd)\n{\n\tint need_in, need_out, need_err;\n\tint fdin[2], fdout[2], fderr[2];\n\tint failed_errno = failed_errno;\n\nI have not found failed_errno as global variable.\nfailed_errno = failed_errno means nothing.\n\nIt is possible coding error and should be\nint failed_errno= 0 or\nfailed_errno=errno.\n"},{"id":"130202","messageId":"2E55A685-834C-4C64-99F1-4060FEC7C2DB@wincent.com","threadId":"21998","inReplyTo":"1976ea660912202246k45732bf2p111bbeb78047693e@mail.gmail.com","subject":"Re: possible code error at run_command.c","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2009-12-21T07:09:36Z","receivedAt":"2009-12-21T07:09:36Z","isPatch":false,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 21/12/2009, a las 07:46, Frank Li escribió:\n\n> int start_command(struct child_process *cmd)\n> {\n> \tint need_in, need_out, need_err;\n> \tint fdin[2], fdout[2], fderr[2];\n> \tint failed_errno = failed_errno;\n>\n> I have not found failed_errno as global variable.\n> failed_errno = failed_errno means nothing.\n>\n> It is possible coding error and should be\n> int failed_errno= 0 or\n> failed_errno=errno.\n\nVia \"git blame\":\n\ncommit 5a7a3671b74c043216549b94a718da04cc3ffcd6\nAuthor: David Soria Parra <dsp@php.net>\nDate:   Tue Aug 4 11:28:40 2009 +0200\n\n     run-command.c: squelch a \"use before assignment\" warning\n\n     i686-apple-darwin9-gcc-4.0.1 (GCC) 4.0.1 (Apple Inc. build 5490)  \ncompiler\n     (and probably others) mistakenly thinks variable failed_errno is  \nused\n     before assigned.  Work it around by giving it a fake  \ninitialization.\n\n     Signed-off-by: David Soria Parra <dsp@php.net>\n     Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\n\nCheers,\nWincent"},{"id":"130201","messageId":"4B2F214D.2020905@viscovery.net","threadId":"21998","inReplyTo":"1976ea660912202246k45732bf2p111bbeb78047693e@mail.gmail.com","subject":"Re: possible code error at run_command.c","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-12-21T07:18:37Z","receivedAt":"2009-12-21T07:18:37Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Frank Li schrieb:\n> int start_command(struct child_process *cmd)\n> {\n> \tint need_in, need_out, need_err;\n> \tint fdin[2], fdout[2], fderr[2];\n> \tint failed_errno = failed_errno;\n> \n> I have not found failed_errno as global variable.\n> failed_errno = failed_errno means nothing.\n\nThis is a commonly used idiom to avoid an (incorrect) compiler warning\nabout an uninitialized variable.\n\nStrictly speaking, I think that you are right by saying \"means nothing\"\nbecause the use of the uninitialized variable invokes undefined behavior\n(and for this reason, I dislike this construct), but in practice it will\nnot make a difference.\n\n-- Hannes\n"},{"id":"130203","messageId":"1976ea660912202329x42f6add3j9175867e8723a4bd@mail.gmail.com","threadId":"21998","inReplyTo":"4B2F214D.2020905@viscovery.net","subject":"Re: possible code error at run_command.c","fromName":"Frank Li","fromEmail":"lznuaa@gmail.com","sentAt":"2009-12-21T07:29:34Z","receivedAt":"2009-12-21T07:29:34Z","isPatch":false,"sender":{"key":"lznuaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40642?v=4"},"body":"> This is a commonly used idiom to avoid an (incorrect) compiler warning\n> about an uninitialized variable.\n>\n> Strictly speaking, I think that you are right by saying \"means nothing\"\n> because the use of the uninitialized variable invokes undefined behavior\n> (and for this reason, I dislike this construct), but in practice it will\n> not make a difference.\n>\n\nThis error is captured at MSVC environment by run time varible check.\nI prefer change it to\nint failed_errno = errno;\n"},{"id":"130204","messageId":"4B2F2727.4060405@viscovery.net","threadId":"21998","inReplyTo":"1976ea660912202329x42f6add3j9175867e8723a4bd@mail.gmail.com","subject":"Re: possible code error at run_command.c","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-12-21T07:43:35Z","receivedAt":"2009-12-21T07:43:35Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Frank Li schrieb:\n>> This is a commonly used idiom to avoid an (incorrect) compiler warning\n>> about an uninitialized variable.\n>>\n>> Strictly speaking, I think that you are right by saying \"means nothing\"\n>> because the use of the uninitialized variable invokes undefined behavior\n>> (and for this reason, I dislike this construct), but in practice it will\n>> not make a difference.\n>>\n> \n> This error is captured at MSVC environment by run time varible check.\n\nDisable this check - it just takes away performance. :-)\n\n(If you don't disable the check, then keep the required changes private.)\n\n> I prefer change it to\n> int failed_errno = errno;\n\nYou don't need to initialize the variable at all because it is always\ninitialized elsewhere before it is used. Perhaps MSVC is clever enough to\nsee it?\n\n-- Hannes\n"},{"id":"130206","messageId":"1976ea660912210018y15acfe32o78841d5e0968f793@mail.gmail.com","threadId":"21998","inReplyTo":"4B2F2727.4060405@viscovery.net","subject":"Re: possible code error at run_command.c","fromName":"Frank Li","fromEmail":"lznuaa@gmail.com","sentAt":"2009-12-21T08:18:26Z","receivedAt":"2009-12-21T08:18:26Z","isPatch":false,"sender":{"key":"lznuaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40642?v=4"},"body":">\n> Disable this check - it just takes away performance. :-)\n\nRelease version will disable this.\nDebug version enable this by default is just for find out code error.\n\n>\n> (If you don't disable the check, then keep the required changes private.)\n>\n>> I prefer change it to\n>> int failed_errno = errno;\n>\n> You don't need to initialize the variable at all because it is always\n> initialized elsewhere before it is used. Perhaps MSVC is clever enough to\n> see it?\n>\n\nMaybe some excute path miss initialized it. Otherwise compiler will\nnot report warning.\n"},{"id":"130207","messageId":"4B2F320A.6010908@viscovery.net","threadId":"21998","inReplyTo":"1976ea660912210018y15acfe32o78841d5e0968f793@mail.gmail.com","subject":"Re: possible code error at run_command.c","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-12-21T08:30:02Z","receivedAt":"2009-12-21T08:30:02Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Frank Li schrieb:\n> Maybe some excute path miss initialized it. Otherwise compiler will\n> not report warning.\n\nLOOK AT THE CODE. There is no such code path.\n\n-- Hannes\n"},{"id":"130208","messageId":"40aa078e0912210041g1fd94c77g6cf9f1b236b6ecd7@mail.gmail.com","threadId":"21998","inReplyTo":"4B2F320A.6010908@viscovery.net","subject":"Re: possible code error at run_command.c","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-12-21T08:41:42Z","receivedAt":"2009-12-21T08:41:42Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Dec 21, 2009 at 9:30 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Frank Li schrieb:\n>> Maybe some excute path miss initialized it. Otherwise compiler will\n>> not report warning.\n>\n> LOOK AT THE CODE. There is no such code path.\n>\n\nThat's odd. I agree, there isn't such a code path. But this is the\nfirst time I've ever seen this MSVC-feature turn up false positives,\nwhich puzzles me.\n\nPerhaps Frank has had to modify start_command for MSVC, and somehow\nintroduced such an error? I dunno.\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"130209","messageId":"4B2F3732.6030903@viscovery.net","threadId":"21998","inReplyTo":"40aa078e0912210041g1fd94c77g6cf9f1b236b6ecd7@mail.gmail.com","subject":"Re: possible code error at run_command.c","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-12-21T08:52:02Z","receivedAt":"2009-12-21T08:52:02Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Erik Faye-Lund schrieb:\n> On Mon, Dec 21, 2009 at 9:30 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n>> Frank Li schrieb:\n>>> Maybe some excute path miss initialized it. Otherwise compiler will\n>>> not report warning.\n>> LOOK AT THE CODE. There is no such code path.\n>>\n> \n> That's odd.\n\nOnly if Frank did the homework and removed the initialization from\n\n\tint failed_errno = failed_errno;\n\n> I agree, there isn't such a code path. But this is the\n> first time I've ever seen this MSVC-feature turn up false positives,\n> which puzzles me.\n\nThis line will trigger the check. It initializes failed_errno with itself,\nwhich is still uninitialized at this time.\n\nNote that we have more definitions of this kind:\n\n$ git grep -E ' ([a-zA-Z_][a-zA-Z_0-9]*) = \\1[;,]' *.c\nbuiltin-rev-list.c:             int reaches = reaches, all = all;\nmatch-trees.c:          unsigned mode1 = mode1;\nmatch-trees.c:          unsigned mode2 = mode2;\nrun-command.c:  int failed_errno = failed_errno;\ntransport.c:            int cmp = cmp, len;\nwt-status.c:    int status = status;\n\n-- Hannes\n"},{"id":"130210","messageId":"40aa078e0912210108y4ea67dbfs2025bae20bdfb381@mail.gmail.com","threadId":"21998","inReplyTo":"4B2F3732.6030903@viscovery.net","subject":"Re: possible code error at run_command.c","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-12-21T09:08:49Z","receivedAt":"2009-12-21T09:08:49Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Dec 21, 2009 at 9:52 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Erik Faye-Lund schrieb:\n>> On Mon, Dec 21, 2009 at 9:30 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n>>> Frank Li schrieb:\n>>>> Maybe some excute path miss initialized it. Otherwise compiler will\n>>>> not report warning.\n>>> LOOK AT THE CODE. There is no such code path.\n>>>\n>>\n>> That's odd.\n>\n> Only if Frank did the homework and removed the initialization from\n>\n>        int failed_errno = failed_errno;\n>\n>> I agree, there isn't such a code path. But this is the\n>> first time I've ever seen this MSVC-feature turn up false positives,\n>> which puzzles me.\n>\n> This line will trigger the check. It initializes failed_errno with itself,\n> which is still uninitialized at this time.\n>\n> Note that we have more definitions of this kind:\n>\n> $ git grep -E ' ([a-zA-Z_][a-zA-Z_0-9]*) = \\1[;,]' *.c\n> builtin-rev-list.c:             int reaches = reaches, all = all;\n> match-trees.c:          unsigned mode1 = mode1;\n> match-trees.c:          unsigned mode2 = mode2;\n> run-command.c:  int failed_errno = failed_errno;\n> transport.c:            int cmp = cmp, len;\n> wt-status.c:    int status = status;\n>\n> -- Hannes\n>\n>\n\nRight, I didn't think of that. Since that is the case, I'd say\ndisabling run-time checks is perfectly sane.\n\nThat being said, it might make sense for MSVC people to be able to\nhave this feature turned on so they can easier find REAL\nprogrammer-errors. But I guess removing the self-assignments makes\ntrouble for non-MSVC people.\n\nPerhaps some of these warnings could be avoided in a \"safer\" way? IMO,\nassigning variables to themselves like this is a bit fishy, as it\neffectively hides uninitialized-use warnings for the entire lifetime\nof that variable. Sure, the programmer who did that probably knows\nwhat he or she does -- but they can't possibly know what people will\ndo in the future. If we could somehow turn these (really few)\noccasions into something that produces reliable warnings on\nuninitialized variables, I think we might be able to catch some bugs\nearlier.\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"130211","messageId":"1976ea660912210113k6a8b05abwf86a0ea4f74aa997@mail.gmail.com","threadId":"21998","inReplyTo":"4B2F3732.6030903@viscovery.net","subject":"Re: possible code error at run_command.c","fromName":"Frank Li","fromEmail":"lznuaa@gmail.com","sentAt":"2009-12-21T09:13:19Z","receivedAt":"2009-12-21T09:13:19Z","isPatch":false,"sender":{"key":"lznuaa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40642?v=4"},"body":"> This line will trigger the check. It initializes failed_errno with itself,\n> which is still uninitialized at this time.\n>\nI see.\nI always use release version at finial production.\nThis is not important. I read code again. There are not path.\nThank you take care.\n"}]}