{"thread":{"id":"34445","subject":"[PATCH] Fix some sparse warnings","startedAt":"2013-07-15T17:31:29Z","lastAt":"2013-07-18T17:58:40Z","messageCount":12,"participants":["Ramsay Jones","Johannes Sixt","Jeff King","Philip Oakley","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"223462","messageId":"51E431F1.6050002@ramsay1.demon.co.uk","threadId":"34445","inReplyTo":null,"subject":"[PATCH] Fix some sparse warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-07-15T17:31:29Z","receivedAt":"2013-07-15T17:31:29Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSparse issues three \"Using plain integer as NULL pointer\" warnings.\nEach warning relates to the use of an '{0}' initialiser expression\nin the declaration of an 'struct object_info'. The first field of\nthis structure has pointer type. Thus, in order to suppress these\nwarnings, we replace the initialiser expression with '{NULL}'.\n\nSigned-off-by: Ramsay Jones \n---\n\nHi Jeff,\n\nIf you need to re-roll the patches in your 'jk/in-pack-size-measurement'\nbranch, could you please squash this (or something like it) into the\npatches equivalent to commit 7c07385d (\"zero-initialize object_info structs\",\n07-07-2013) [sha1_file.c and streaming.c] and commit 778d263a (\"cat-file: add\n--batch-disk-sizes option\", 07-07-2013) [builtin/cat-file.c].\n\nThanks!\n\nATB,\nRamsay Jones\n\n\n builtin/cat-file.c | 2 +-\n sha1_file.c | 2 +-\n streaming.c | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex bf12883..860576e 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -135,7 +135,7 @@ static int batch_one_object(const char *obj_name, int print_contents)\n \tif (print_contents == BATCH)\n \t\tcontents = read_sha1_file(sha1, &type, &size);\n \telse if (print_contents == BATCH_DISK_SIZES) {\n-\t\tstruct object_info oi = {0};\n+\t\tstruct object_info oi = {NULL};\n \t\toi.disk_sizep = &size;\n \t\ttype = sha1_object_info_extended(sha1, &oi);\n \t}\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4c2365f..e4ab0a4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2440,7 +2440,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)\n \n int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)\n {\n-\tstruct object_info oi = {0};\n+\tstruct object_info oi = {NULL};\n \n \toi.sizep = sizep;\n \treturn sha1_object_info_extended(sha1, &oi);\ndiff --git a/streaming.c b/streaming.c\nindex cac282f..5710065 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -135,7 +135,7 @@ struct git_istream *open_istream(const unsigned char *sha1,\n \t\t\t\t struct stream_filter *filter)\n {\n \tstruct git_istream *st;\n-\tstruct object_info oi = {0};\n+\tstruct object_info oi = {NULL};\n \tconst unsigned char *real = lookup_replace_object(sha1);\n \tenum input_source src = istream_source(real, type, &oi);\n \n-- \n1.8.3\n"},{"id":"223500","messageId":"51E4E0C0.3060604@viscovery.net","threadId":"34445","inReplyTo":"51E431F1.6050002@ramsay1.demon.co.uk","subject":"Re: [PATCH] Fix some sparse warnings","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-07-16T05:57:20Z","receivedAt":"2013-07-16T05:57:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 7/15/2013 19:31, schrieb Ramsay Jones:\n> Sparse issues three \"Using plain integer as NULL pointer\" warnings.\n> Each warning relates to the use of an '{0}' initialiser expression\n> in the declaration of an 'struct object_info'.\n\nI question the value of this warning. Initialization with '= {0}' is a\nwell-established idiom, and sparse should know about it. Also, plain 0\n*is* a null pointer constant.\n\n-- Hannes\n"},{"id":"223502","messageId":"20130716062122.GA4964@sigill.intra.peff.net","threadId":"34445","inReplyTo":"51E4E0C0.3060604@viscovery.net","subject":"Re: [PATCH] Fix some sparse warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-16T06:21:22Z","receivedAt":"2013-07-16T06:21:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:\n\n> Am 7/15/2013 19:31, schrieb Ramsay Jones:\n> > Sparse issues three \"Using plain integer as NULL pointer\" warnings.\n> > Each warning relates to the use of an '{0}' initialiser expression\n> > in the declaration of an 'struct object_info'.\n> \n> I question the value of this warning. Initialization with '= {0}' is a\n> well-established idiom, and sparse should know about it. Also, plain 0\n> *is* a null pointer constant.\n\nI agree with you. It's not a bug, and I think sparse is being overly\npicky here; it is missing the forest for the trees in interpreting the\nidiom.\n\nStill, it may be worth tweaking in the name of eliminating compiler\nnoise, since it does not cost us very much to do so (and I believe we\nhave done so in the past, too).\n\nWe could also ask people with sparse to turn off the \"use NULL instead\nof 0\" warning, but I think it _is_ a useful warning elsewhere (even\nthough it is never a bug, it violates our style guidelines and may be an\nindication of a bug). It would be nice if sparse learned to ignore the\nwarning in this particular idiom, but I am not going to hold my breath\nfor that.\n\n-Peff\n"},{"id":"223543","messageId":"6BDA2E3E7318418BBB2C19B475B2B118@PhilipOakley","threadId":"34445","inReplyTo":"20130716062122.GA4964@sigill.intra.peff.net","subject":"Re: [PATCH] Fix some sparse warnings","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2013-07-16T20:53:35Z","receivedAt":"2013-07-16T20:53:35Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" \nSent: Tuesday, July 16, 2013 7:21 AM\n> On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:\n>\n>> Am 7/15/2013 19:31, schrieb Ramsay Jones:\n>> > Sparse issues three \"Using plain integer as NULL pointer\" warnings.\n>> > Each warning relates to the use of an '{0}' initialiser expression\n>> > in the declaration of an 'struct object_info'.\n>>\n>> I question the value of this warning. Initialization with '= {0}' is\n>> a\n>> well-established idiom, and sparse should know about it. Also, plain\n>> 0\n>> *is* a null pointer constant.\n>\n> I agree with you. It's not a bug, and I think sparse is being overly\n> picky here; it is missing the forest for the trees in interpreting the\n> idiom.\n>\n> Still, it may be worth tweaking in the name of eliminating compiler\n> noise, since it does not cost us very much to do so (and I believe we\n> have done so in the past, too).\n>\n> We could also ask people with sparse to turn off the \"use NULL instead\n> of 0\" warning, but I think it _is_ a useful warning elsewhere (even\n> though it is never a bug, it violates our style guidelines and may be\n> an\n> indication of a bug). It would be nice if sparse learned to ignore the\n> warning in this particular idiom, but I am not going to hold my breath\n> for that.\n>\n> -Peff\n> --\n\nOn the subject of warnings and null pointers, yesterday's Code Project\nnews linked to a blog on the problems of unexpected optimization bugs,\nsuch as dereferencing a null pointer. \"Finding Undefined Behavior Bugs\nby Finding Dead Code\" http://blog.regehr.org/archives/970 which links to \nthe draft of an interesting paper \n[http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf]\n\nDoes anyone run the \"new static checker called 'Stack' that precisely\nidentifies unstable code\"? [though the paper's conclusion says 'All\nStack source code will be publicly available.' which suggests it's not\nyet available]\n\nOr use the ‘-fno-delete-null-pointer-checks’ referred to in the blog\ncomments (see also index : kernel/git/torvalds/linux.git \"Add\n'-fno-delete-null-pointer-checks' to gcc CFLAGS\"\nhttps://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a3ca86aea507904148870946d599e07a340b39bf\n\nYou're probably already aware of these aspects but I thought it worth\nmentioning for the wider readership.\n\nregards\n\nPhilip\n"},{"id":"223545","messageId":"51E5B8BD.8090202@googlemail.com","threadId":"34445","inReplyTo":"6BDA2E3E7318418BBB2C19B475B2B118@PhilipOakley","subject":"Re: [PATCH] Fix some sparse warnings","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-16T21:18:53Z","receivedAt":"2013-07-16T21:18:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 07/16/2013 10:53 PM, Philip Oakley wrote:\n> From: \"Jeff King\" \n> Sent: Tuesday, July 16, 2013 7:21 AM\n>> On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:\n>>\n>>> Am 7/15/2013 19:31, schrieb Ramsay Jones:\n>>> > Sparse issues three \"Using plain integer as NULL pointer\" warnings.\n>>> > Each warning relates to the use of an '{0}' initialiser expression\n>>> > in the declaration of an 'struct object_info'.\n>>>\n>>> I question the value of this warning. Initialization with '= {0}' is\n>>> a\n>>> well-established idiom, and sparse should know about it. Also, plain\n>>> 0\n>>> *is* a null pointer constant.\n>>\n>> I agree with you. It's not a bug, and I think sparse is being overly\n>> picky here; it is missing the forest for the trees in interpreting the\n>> idiom.\n>>\n>> Still, it may be worth tweaking in the name of eliminating compiler\n>> noise, since it does not cost us very much to do so (and I believe we\n>> have done so in the past, too).\n>>\n>> We could also ask people with sparse to turn off the \"use NULL instead\n>> of 0\" warning, but I think it _is_ a useful warning elsewhere (even\n>> though it is never a bug, it violates our style guidelines and may be\n>> an\n>> indication of a bug). It would be nice if sparse learned to ignore the\n>> warning in this particular idiom, but I am not going to hold my breath\n>> for that.\n>>\n>> -Peff\n>> -- \n> \n> On the subject of warnings and null pointers, yesterday's Code Project\n> news linked to a blog on the problems of unexpected optimization bugs,\n> such as dereferencing a null pointer. \"Finding Undefined Behavior Bugs\n> by Finding Dead Code\" http://blog.regehr.org/archives/970 which links to\n> the draft of an interesting paper\n> [http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf]\n> \n> Does anyone run the \"new static checker called 'Stack' that precisely\n> identifies unstable code\"? [though the paper's conclusion says 'All\n> Stack source code will be publicly available.' which suggests it's not\n> yet available]\n> \n> Or use the ‘-fno-delete-null-pointer-checks’ referred to in the blog\n> comments (see also index : kernel/git/torvalds/linux.git \"Add\n> '-fno-delete-null-pointer-checks' to gcc CFLAGS\"\n> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a3ca86aea507904148870946d599e07a340b39bf\n> \n> \n> You're probably already aware of these aspects but I thought it worth\n> mentioning for the wider readership.\n> \n> regards\n> \n> Philip\n> \n> \n\nI recently started contributing and I used cppcheck found at\nhttps://github.com/danmar/cppcheck to submit the patches\nat origin/sb/misc-fixes\nAs it is originally for C++, that tool throws lots of\nfalse-positives (i.e. warns about null pointer dereferencing\nwhen it's not possible to be a null pointer) unfortunately.\n\nAlso I hear llvm/clang comes with a good static code analyzer,\nwhich I tried today on a different project.\nThough I could not figure out how to use that on a pure C project\nsuch as git, as that tool seems to require a C++ compilation for\ndoing its static code analysis.\n\nThe blog post you linked to seems very interesting and promising. :)\n\nRegards,\nStefan\n\n\n"},{"id":"223552","messageId":"51E5C6D1.7000505@iee.org","threadId":"34445","inReplyTo":"51E5B8BD.8090202@googlemail.com","subject":"Re: [PATCH] Fix some sparse warnings","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2013-07-16T22:18:57Z","receivedAt":"2013-07-16T22:18:57Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 16/07/13 22:18, Stefan Beller wrote:\n> On 07/16/2013 10:53 PM, Philip Oakley wrote:\n>> From: \"Jeff King\" \n>> Sent: Tuesday, July 16, 2013 7:21 AM\n>>> On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:\n>>>\n>>>> Am 7/15/2013 19:31, schrieb Ramsay Jones:\n>>>>> Sparse issues three \"Using plain integer as NULL pointer\" warnings.\n>>>>> Each warning relates to the use of an '{0}' initialiser expression\n>>>>> in the declaration of an 'struct object_info'.\n>>>>\n>>>> I question the value of this warning. Initialization with '= {0}' is\n>>>> a\n>>>> well-established idiom, and sparse should know about it. Also, plain\n>>>> 0\n>>>> *is* a null pointer constant.\n>>>\n>>> I agree with you. It's not a bug, and I think sparse is being overly\n>>> picky here; it is missing the forest for the trees in interpreting the\n>>> idiom.\n>>>\n>>> Still, it may be worth tweaking in the name of eliminating compiler\n>>> noise, since it does not cost us very much to do so (and I believe we\n>>> have done so in the past, too).\n>>>\n>>> We could also ask people with sparse to turn off the \"use NULL instead\n>>> of 0\" warning, but I think it _is_ a useful warning elsewhere (even\n>>> though it is never a bug, it violates our style guidelines and may be\n>>> an\n>>> indication of a bug). It would be nice if sparse learned to ignore the\n>>> warning in this particular idiom, but I am not going to hold my breath\n>>> for that.\n>>>\n>>> -Peff\n>>> --\n>>\n>> On the subject of warnings and null pointers, yesterday's Code Project\n>> news linked to a blog on the problems of unexpected optimization bugs,\n>> such as dereferencing a null pointer. \"Finding Undefined Behavior Bugs\n>> by Finding Dead Code\" http://blog.regehr.org/archives/970 which links to\n>> the draft of an interesting paper\n>> [http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf]\n>>\n>> Does anyone run the \"new static checker called 'Stack' that precisely\n>> identifies unstable code\"? [though the paper's conclusion says 'All\n>> Stack source code will be publicly available.' which suggests it's not\n>> yet available]\n>>\n>> Or use the ‘-fno-delete-null-pointer-checks’ referred to in the blog\n>> comments (see also index : kernel/git/torvalds/linux.git \"Add\n>> '-fno-delete-null-pointer-checks' to gcc CFLAGS\"\n>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a3ca86aea507904148870946d599e07a340b39bf\n>>\n>>\n>> You're probably already aware of these aspects but I thought it worth\n>> mentioning for the wider readership.\n>>\n>> regards\n>>\n>> Philip\n>>\n>>\n>\n> I recently started contributing and I used cppcheck found at\n> https://github.com/danmar/cppcheck to submit the patches\n> at origin/sb/misc-fixes\n> As it is originally for C++, that tool throws lots of\n> false-positives (i.e. warns about null pointer dereferencing\n> when it's not possible to be a null pointer) unfortunately.\n>\n> Also I hear llvm/clang comes with a good static code analyzer,\n> which I tried today on a different project.\n> Though I could not figure out how to use that on a pure C project\n> such as git, as that tool seems to require a C++ compilation for\n> doing its static code analysis.\n>\n> The blog post you linked to seems very interesting and promising. :)\n>\n> Regards,\n> Stefan\n>\nA bit more searching also finds \nhttp://pdos.csail.mit.edu/~xi/papers/ub-apsys12.pdf and \nhttp://pdos.csail.mit.edu/~xi/papers/ub-apsys12-slides.pptx by the same \nauthor.\n"},{"id":"223560","messageId":"51E62FF4.60809@viscovery.net","threadId":"34445","inReplyTo":"51E5B8BD.8090202@googlemail.com","subject":"Re: [PATCH] Fix some sparse warnings","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-07-17T05:47:32Z","receivedAt":"2013-07-17T05:47:32Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":">>>> I question the value of this warning. Initialization with '=\n>>>> {0}' is a well-established idiom, and sparse should know about\n>>>> it.\n\nThanks everyone for your feedback. But I really wanted to call only the\nwarning in the case of the '= {0}' idiom into question, not about 0 vs.\nNULL in general.\n\n-- Hannes\n"},{"id":"223617","messageId":"51E715D8.9040307@googlemail.com","threadId":"34445","inReplyTo":"6BDA2E3E7318418BBB2C19B475B2B118@PhilipOakley","subject":"Re: [PATCH] Fix some sparse warnings","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-17T22:08:24Z","receivedAt":"2013-07-17T22:08:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 07/16/2013 10:53 PM, Philip Oakley wrote:\n> \n> Does anyone run the \"new static checker called 'Stack' that precisely\n> identifies unstable code\"? [though the paper's conclusion says 'All\n> Stack source code will be publicly available.' which suggests it's not\n> yet available]\n> \n\nSo I started using the clang code analyzer on git. One of the \nfirst warnings actually is this:\n\nobject.c:241:7: warning: Branch condition evaluates to a garbage value\n if (!eaten)\n\nSo that part of object.c lookx like this:\n\tstruct object *parse_object(const unsigned char *sha1) \n\t{\n\t\tint eaten;\n\t\t...\n\t\tobj = parse_object_buffer(sha1, type, size, buffer, &eaten);\n\t\tif (!eaten)\n\t\t\tfree(buffer);\n\t}\n\nAnd the parse_object_buffer looks like this with respect to the eaten \nvariable:\n\tstruct object *parse_object_buffer(...)\n\t{\n\t\tint eaten = 0;\n\t\tif (something)\n\t\t\treturn NULL;\n\t\t...\n\t\tif (something_different)\n\t\t\teaten=1;\n\t\t*eaten_p = eaten;\n\t}\n\nSo what might happen is, that parse_object_buffer exits early, without\nexecuting \n\t\n\t*eaten_p = eaten;\n\nThen in the parse_object function eaten was never initialized nor set \ninside the call to parse_object_buffer. Then it is obvious that the\nfree(buffer) is executed depending on garbage left on the stack.\nDefinitely something what we want to change.\n\nThe obvious way to repair this would be to just initialize the eaten variable\ninside parse_object.\n\tstruct object *parse_object(const unsigned char *sha1) \n\t{\n\t\tint eaten=0;\n\t\t...\n\nHowever I'd like to propose another solution:\nIn parse_object_buffer we do not have a local eaten variable, but\ndirectly write to *eaten_p. That would be the following patch.\n\nWas there a particular idea or goal behind first having a local eaten\nvariable, which later near the correct return of the function was used to set the \neaten_p?\n\nThanks,\nStefan\n\n\n\n"},{"id":"223618","messageId":"1374098982-6962-1-git-send-email-stefanbeller@googlemail.com","threadId":"34445","inReplyTo":"51E715D8.9040307@googlemail.com","subject":"[PATCH] parse_object_buffer: Correct freeing the buffer.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-17T22:09:42Z","receivedAt":"2013-07-17T22:09:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If we exit early in the function parse_object_buffer, we did not\nwrite to *eaten_p. Then the calling function parse_object, which looks\nlike the following with respect to the eaten variable, cannot rely on a\nproper value set in eaten, hence the freeing of the buffer depends\non random values in memory.\n\n\tstruct object *parse_object(const unsigned char *sha1)\n\t{\n\t\tint eaten;\n\t\t...\n\t\tobj = parse_object_buffer(sha1, type, size, buffer, &eaten);\n\t\tif (!eaten)\n\t\t\tfree(buffer);\n\t}\n\nThis change makes sure, the buffer freeing condition is deterministic.\n\nSigned-off-by: Stefan Beller \n---\n object.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex cbc7333..d8a4b1f 100644\n--- a/object.c\n+++ b/object.c\n@@ -145,7 +145,7 @@ struct object *lookup_unknown_object(const unsigned char *sha1)\n struct object *parse_object_buffer(const unsigned char *sha1, enum object_type type, unsigned long size, void *buffer, int *eaten_p)\n {\n \tstruct object *obj;\n-\tint eaten = 0;\n+\t*eaten_p = 0;\n \n \tobj = NULL;\n \tif (type == OBJ_BLOB) {\n@@ -164,7 +164,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\t\tif (!tree->object.parsed) {\n \t\t\t\tif (parse_tree_buffer(tree, buffer, size))\n \t\t\t\t\treturn NULL;\n-\t\t\t\teaten = 1;\n+\t\t\t\t*eaten_p = 1;\n \t\t\t}\n \t\t}\n \t} else if (type == OBJ_COMMIT) {\n@@ -174,7 +174,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\t\t\treturn NULL;\n \t\t\tif (!commit->buffer) {\n \t\t\t\tcommit->buffer = buffer;\n-\t\t\t\teaten = 1;\n+\t\t\t\t*eaten_p = 1;\n \t\t\t}\n \t\t\tobj = &commit->object;\n \t\t}\n@@ -191,7 +191,6 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t}\n \tif (obj && obj->type == OBJ_NONE)\n \t\tobj->type = type;\n-\t*eaten_p = eaten;\n \treturn obj;\n }\n \n-- \n1.8.3.3.754.g9c3c367.dirty\n"},{"id":"223620","messageId":"51E717C4.6010606@googlemail.com","threadId":"34445","inReplyTo":"51E715D8.9040307@googlemail.com","subject":"Re: [PATCH] Fix some sparse warnings","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-17T22:16:36Z","receivedAt":"2013-07-17T22:16:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 07/18/2013 12:08 AM, Stefan Beller wrote:\n> \n> So I started using the clang code analyzer on git. One of the \n> first warnings actually is this:\n> \n\nSo in case somebody else would also like to play around with the\nclang static code analyzer:\n\n# get clang:\ncd \ngit clone http://llvm.org/git/llvm.git\ncd llvm/tools\ngit clone http://llvm.org/git/clang\ncd \nmkdir build && cd build\ncmake ..\n# if desired make install\n\n# in the Makefile of git change\nCC = /llvm/tools/clang/tools/scan-build/ccc-analyzer\n# then obviously:\nmake clean\nmake\n\n"},{"id":"223621","messageId":"7vd2qg6abf.fsf@alter.siamese.dyndns.org","threadId":"34445","inReplyTo":"51E715D8.9040307@googlemail.com","subject":"Re: [PATCH] Fix some sparse warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-17T23:22:44Z","receivedAt":"2013-07-17T23:22:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> And the parse_object_buffer looks like this with respect to the eaten \n> variable:\n> \tstruct object *parse_object_buffer(...)\n> \t{\n> \t\tint eaten = 0;\n> \t\tif (something)\n> \t\t\treturn NULL;\n> \t\t...\n> \t\tif (something_different)\n> \t\t\teaten=1;\n> \t\t*eaten_p = eaten;\n> \t}\n> ...\n> Was there a particular idea or goal behind first having a local eaten\n> variable, which later near the correct return of the function was used to set the \n> eaten_p?\n\nI didn't run \"blame\" to see the evolution of this function, but I\nsuspect that the original code, when the \"eaten\" local variable was\nintroduced, very much tried to do exactly what you suspect. The\nearly return codepaths you see in today's code may be much newer,\nadded without much thinking about the exact issue you are bringing\nup.\n"},{"id":"223690","messageId":"51E82CD0.8060302@ramsay1.demon.co.uk","threadId":"34445","inReplyTo":"20130716062122.GA4964@sigill.intra.peff.net","subject":"Re: [PATCH] Fix some sparse warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-07-18T17:58:40Z","receivedAt":"2013-07-18T17:58:40Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jeff King wrote:\n> On Tue, Jul 16, 2013 at 07:57:20AM +0200, Johannes Sixt wrote:\n> \n>> Am 7/15/2013 19:31, schrieb Ramsay Jones:\n>>> Sparse issues three \"Using plain integer as NULL pointer\" warnings.\n>>> Each warning relates to the use of an '{0}' initialiser expression\n>>> in the declaration of an 'struct object_info'.\n>>\n>> I question the value of this warning. Initialization with '= {0}' is a\n>> well-established idiom, and sparse should know about it. Also, plain 0\n>> *is* a null pointer constant.\n> \n> I agree with you. It's not a bug, and I think sparse is being overly\n> picky here; it is missing the forest for the trees in interpreting the\n> idiom.\n\nYes, last time this came up, I looked at writing a patch to sparse to\nallow this without complaint. It's still on my sparse TODO list, but\neven if I finished it tonight, it would take a while to get into a\nreleased version of sparse. ;-)\n\n> Still, it may be worth tweaking in the name of eliminating compiler\n> noise, since it does not cost us very much to do so (and I believe we\n> have done so in the past, too).\n> \n> We could also ask people with sparse to turn off the \"use NULL instead\n> of 0\" warning, but I think it _is_ a useful warning elsewhere (even\n> though it is never a bug, it violates our style guidelines and may be an\n> indication of a bug).\n\nIndeed, if it wasn't for this, I would be happy to turn this warning off.\n\nATB,\nRamsay Jones\n"}]}