{"thread":{"id":"28860","subject":"[PATCH 0/3] Fix code issues spotted by clang","startedAt":"2011-11-06T12:06:21Z","lastAt":"2011-11-08T16:05:57Z","messageCount":22,"participants":["Ævar Arnfjörð Bjarmason","Andreas Schwab","Junio C Hamano","Jeff King","Giuseppe Bilotta","Jonathan Nieder","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"178959","messageId":"1320581184-4557-1-git-send-email-avarab@gmail.com","threadId":"28860","inReplyTo":null,"subject":"[PATCH 0/3] Fix code issues spotted by clang","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-11-06T12:06:21Z","receivedAt":"2011-11-06T12:06:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series fixes some of the code issues raised by clang. This leaves\nthe following warnings that I haven't addressed:\n\n revision.c:766:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967279 to 134217711\n [-Wconstant-conversion]\n p->item->object.flags &= ~TMP_MARK;\n ^ ~~~~~~~~~\n revision.c:768:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967279 to 134217711\n [-Wconstant-conversion]\n p->item->object.flags &= ~TMP_MARK;\n ^ ~~~~~~~~~\n revision.c:1875:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967279 to 134217711\n [-Wconstant-conversion]\n p->item->object.flags &= ~TMP_MARK;\n ^ ~~~~~~~~~\n revision.c:2202:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967158 to 134217590\n [-Wconstant-conversion]\n commit->object.flags &= ~(ADDED | SEEN | SHOWN);\n ^ ~~~~~~~~~~~~~~~~~~~~~~~\n upload-pack.c:115:12: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967293 to 134217725\n [-Wconstant-conversion]\n o->flags &= ~UNINTERESTING;\n ^ ~~~~~~~~~~~~~~\n upload-pack.c:689:19: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294705151 to 133955583\n [-Wconstant-conversion]\n object->flags &= ~CLIENT_SHALLOW;\n ^ ~~~~~~~~~~~~~~~\n builtin/checkout.c:676:16: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967293 to 134217725\n [-Wconstant-conversion]\n object->flags &= ~UNINTERESTING;\n ^ ~~~~~~~~~~~~~~\n builtin/reflog.c:173:32: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294965247 to 134215679\n [-Wconstant-conversion]\n found.objects[i].item->flags &= ~STUDYING;\n ^ ~~~~~~~~~\n builtin/reflog.c:232:31: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294963199 to 134213631\n [-Wconstant-conversion]\n pending->item->object.flags &= ~REACHABLE;\n ^ ~~~~~~~~~~\n bisect.c:66:24: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294901759 to 134152191\n [-Wconstant-conversion]\n commit->object.flags &= ~COUNTED;\n ^ ~~~~~~~~\n \nÆvar Arnfjörð Bjarmason (3):\n apply: get rid of useless x < 0 comparison on a size_t type\n diff/apply: cast variable in call to free()\n grep: get rid of useless x < 0 comparison on an enum member\n\n builtin/apply.c | 3 ---\n builtin/diff.c | 2 +-\n grep.c | 2 +-\n submodule.c | 2 +-\n 4 files changed, 3 insertions(+), 6 deletions(-)\n\n-- \n1.7.6.3\n"},{"id":"178961","messageId":"1320581184-4557-2-git-send-email-avarab@gmail.com","threadId":"28860","inReplyTo":"1320581184-4557-1-git-send-email-avarab@gmail.com","subject":"[PATCH 1/3] apply: get rid of useless x < 0 comparison on a size_t type","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-11-06T12:06:22Z","receivedAt":"2011-11-06T12:06:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"According to the C standard size_t is always unsigned, therefore the\ncomparison \"n1 < 0 || n2 < 0\" when n1 and n2 are size_t will always be\nfalse.\n\nThis was raised by clang 2.9 which throws this warning when compiling\napply.c:\n\n builtin/apply.c:253:9: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]\n if (n1 < 0 || n2 < 0)\n ~~ ^ ~\n builtin/apply.c:253:19: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]\n if (n1 < 0 || n2 < 0)\n ~~ ^ ~\n\nThis check was originally added in v1.6.5-rc0~53^2 by Giuseppe Bilotta\nwhile adding an option to git-apply to ignore whitespace differences.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/apply.c | 3 ---\n 1 files changed, 0 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 84a8a0b..b3b59db 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -250,9 +250,6 @@ static int fuzzy_matchlines(const char *s1, size_t n1,\n \tconst char *last2 = s2 + n2 - 1;\n \tint result = 0;\n \n-\tif (n1 < 0 || n2 < 0)\n-\t\treturn 0;\n-\n \t/* ignore line endings */\n \twhile ((*last1 == '\\r') || (*last1 == '\\n'))\n \t\tlast1--;\n-- \n1.7.6.3\n"},{"id":"178960","messageId":"1320581184-4557-3-git-send-email-avarab@gmail.com","threadId":"28860","inReplyTo":"1320581184-4557-1-git-send-email-avarab@gmail.com","subject":"[PATCH 2/3] diff/apply: cast variable in call to free()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-11-06T12:06:23Z","receivedAt":"2011-11-06T12:06:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Both of these free() calls are freeing a \"const unsigned char (*)[20]\"\ntype while free() expects a \"void *\". This results in the following\nwarning under clang 2.9:\n\n builtin/diff.c:185:7: warning: passing 'const unsigned char (*)[20]' to parameter of type 'void *' discards qualifiers\n free(parent);\n ^~~~~~\n\n submodule.c:394:7: warning: passing 'const unsigned char (*)[20]' to parameter of type 'void *' discards qualifiers\n free(parents);\n ^~~~~~~\n\nThis free()-ing without a cast was added by Jim Meyering to\nbuiltin/diff.c in v1.7.6-rc3~4 and later by Fredrik Gustafsson in\nsubmodule.c in v1.7.7-rc1~25^2.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/diff.c | 2 +-\n submodule.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 1118689..0fe638f 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -182,7 +182,7 @@ static int builtin_diff_combined(struct rev_info *revs,\n \t\thashcpy((unsigned char *)(parent + i), ent[i].item->sha1);\n \tdiff_tree_combined(parent[0], parent + 1, ents - 1,\n \t\t\t revs->dense_combined_merges, revs);\n-\tfree(parent);\n+\tfree((void *)parent);\n \treturn 0;\n }\n \ndiff --git a/submodule.c b/submodule.c\nindex 0fd10a0..52cdcc6 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -391,7 +391,7 @@ static void commit_need_pushing(struct commit *commit, struct commit_list *paren\n \trev.diffopt.format_callback_data = needs_pushing;\n \tdiff_tree_combined(commit->object.sha1, parents, n, 1, &rev);\n \n-\tfree(parents);\n+\tfree((void *)parents);\n }\n \n int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)\n-- \n1.7.6.3\n"},{"id":"178962","messageId":"1320581184-4557-4-git-send-email-avarab@gmail.com","threadId":"28860","inReplyTo":"1320581184-4557-1-git-send-email-avarab@gmail.com","subject":"[PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-11-06T12:06:24Z","receivedAt":"2011-11-06T12:06:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove an \"p->field < 0\" comparison in grep.c that'll always be\nfalse. In this case \"p\" is a \"grep_pat\" where \"field\" is defined as:\n\n\tenum grep_header_field field;\n\nAnd grep_header_field is in turn defined as:\n\n enum grep_header_field {\n \tGREP_HEADER_AUTHOR = 0,\n \tGREP_HEADER_COMMITTER\n };\n\nMeaning that this comparison will always be false. This was spotted by\nclang 2.9 which produced the following warning while compiling grep.c:\n\n grep.c:330:16: warning: comparison of unsigned enum expression < 0 is always false [-Wtautological-compare]\n if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)\n ~~~~~~~~ ^ ~\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n grep.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex b29d09c..025d3f8 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -327,7 +327,7 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)\n \tfor (p = opt->header_list; p; p = p->next) {\n \t\tif (p->token != GREP_PATTERN_HEAD)\n \t\t\tdie(\"bug: a non-header pattern in grep header list.\");\n-\t\tif (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)\n+\t\tif (GREP_HEADER_FIELD_MAX <= p->field)\n \t\t\tdie(\"bug: unknown header field %d\", p->field);\n \t\tcompile_regexp(p, opt);\n \t}\n-- \n1.7.6.3\n"},{"id":"178963","messageId":"CACBZZX7bzFy_NCr4RTpYXXVmieaV7GrBfJgwdtCT1SwoXV_cGA@mail.gmail.com","threadId":"28860","inReplyTo":"1320581184-4557-1-git-send-email-avarab@gmail.com","subject":"Re: [PATCH 0/3] Fix code issues spotted by clang","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-11-06T12:33:47Z","receivedAt":"2011-11-06T12:33:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Nov 6, 2011 at 13:06, Ævar Arnfjörð Bjarmason wrote:\n> This series fixes some of the code issues raised by clang.\n\nOut of curiosity I also compiled git on \"Sun Studio 12 Update 1\" on\nee6dfb2d83ba1b057943e705f707fa27e34e47f9 with this patch series\napplied and it emits the following warnings, quoted below along with\nmy comments:\n\n \"pack-write.c\", line 76: warning: name redefined by pragma\nredefine_extname declared static: tmpfile\n\nPresumably this means that we've imported the function tmpfile() and\nSun's CC doesn't like that we're creating a variable with that name.\n\n \"read-cache.c\", line 761: warning: statement not reached\n\nSunCC's brain is melting on this particularly clever piece of code:\n\n \tgoto inside;\n \tfor (;;) {\n \t\tif (!c)\n \t\t\treturn 1;\n \t\tif (is_dir_sep(c)) {\n inside:\n\n \"sha1_file.c\", line 2453: warning: name redefined by pragma\nredefine_extname declared static: tmpfile\n\nsame tempfile issue.\n\n \"compat/../git-compat-util.h\", line 4: warning: macro redefined:\n_FILE_OFFSET_BITS\n \"compat/../git-compat-util.h\", line 4: warning: macro redefined:\n_FILE_OFFSET_BITS\n\nSeems we're clashing with this:\n\n $ grep -r '#define.*_FILE_OFFSET_BITS' /usr/include/\n /usr/include/sys/feature_tests.h:#define _FILE_OFFSET_BITS 64\n /usr/include/sys/feature_tests.h:#define _FILE_OFFSET_BITS 32\n\n \"xdiff/xutils.c\", line 194: warning: statement not reached\n\nThis was added in b97e911643341cb31e6b97029b9ffd96fc675b1d as a\nworkaround on systems using glibc, should this even be run on Sun\nlibc? Martin?\n\nAnother clever bit of code blowing SunCC's brain:\n\n\tif (flags & XDF_IGNORE_WHITESPACE) {\n\t\tgoto skip_ws;\n\t\twhile (i1 < s1 && i2 < s2) {\n\t\t\tif (l1[i1++] != l2[i2++])\n\t\t\t\treturn 0;\n\t\tskip_ws:\n\t\t\twhile (i1 < s1 && XDL_ISSPACE(l1[i1]))\n\t\t\t\ti1++;\n\t\t\twhile (i2 < s2 && XDL_ISSPACE(l2[i2]))\n\t\t\t\ti2++;\n\t\t}\n\t}\n\n \"fast-import.c\", line 858: warning: name redefined by pragma\nredefine_extname declared static: tmpfile\n\nditto tempfile issue.\n\n \"builtin/fast-export.c\", line 54: warning: enum type mismatch: op \"=\"\n\nThis seems to me to be a legitimate issue we introduced in\n2d8ad46919213ebbd7bb72eb5b56cca8cc3ae07f, Elijah?\n\nWe're defining an enum like this:\n\n static enum { ABORT, VERBATIM, WARN, STRIP } signed_tag_mode = ABORT;\n static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ABORT;\n\nAnd then doing:\n\n\tif (unset || !strcmp(arg, \"abort\"))\n\t\ttag_of_filtered_mode = ABORT; <-- Line 54\n\telse if (!strcmp(arg, \"drop\"))\n\t\ttag_of_filtered_mode = DROP;\n\telse if (!strcmp(arg, \"rewrite\"))\n\t\ttag_of_filtered_mode = REWRITE;\n\nPresumably that assignment should be \"= ERROR\".\n\n \"builtin/index-pack.c\", line 175: warning: name redefined by\npragma redefine_extname declared static: tmpfile\n\nditto tempfile.\n\n \"vcs-svn/string_pool.c\", line 11: warning: initializer will be\nsign-extended: -1\n \"vcs-svn/string_pool.c\", line 81: warning: initializer will be\nsign-extended: -1\n \"vcs-svn/repo_tree.c\", line 112: warning: initializer will be\nsign-extended: -1\n \"vcs-svn/repo_tree.c\", line 112: warning: initializer will be\nsign-extended: -1\n \"test-treap.c\", line 34: warning: initializer will be sign-extended: -1\n\nAll of these come down to assigning ~0 to an uint32_t variable, e.g.:\n\n\tuint32_t token = ~0;\n"},{"id":"178966","messageId":"m2pqh5nvic.fsf@igel.home","threadId":"28860","inReplyTo":"1320581184-4557-4-git-send-email-avarab@gmail.com","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-11-06T15:03:23Z","receivedAt":"2011-11-06T15:03:23Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Remove an \"p->field < 0\" comparison in grep.c that'll always be\n> false. In this case \"p\" is a \"grep_pat\" where \"field\" is defined as:\n>\n> \tenum grep_header_field field;\n>\n> And grep_header_field is in turn defined as:\n>\n> enum grep_header_field {\n> \tGREP_HEADER_AUTHOR = 0,\n> \tGREP_HEADER_COMMITTER\n> };\n>\n> Meaning that this comparison will always be false.\n\nThe underlying integer type is implementation-defined, and can be any\nsigned or unsigned integer type that can represent all enumerations.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"178974","messageId":"7vsjm15cap.fsf@alter.siamese.dyndns.org","threadId":"28860","inReplyTo":"1320581184-4557-2-git-send-email-avarab@gmail.com","subject":"Re: [PATCH 1/3] apply: get rid of useless x < 0 comparison on a size_t type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-06T18:35:42Z","receivedAt":"2011-11-06T18:35:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> According to the C standard size_t is always unsigned, therefore the\n> comparison \"n1 < 0 || n2 < 0\" when n1 and n2 are size_t will always be\n> false.\n>\n> This was raised by clang 2.9 which throws this warning when compiling\n> apply.c:\n>\n> builtin/apply.c:253:9: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]\n> if (n1 < 0 || n2 < 0)\n> ~~ ^ ~\n> builtin/apply.c:253:19: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]\n> if (n1 < 0 || n2 < 0)\n> ~~ ^ ~\n>\n> This check was originally added in v1.6.5-rc0~53^2 by Giuseppe Bilotta\n> while adding an option to git-apply to ignore whitespace differences.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n\nI agree that these quantities can never be negative, so I'll apply the\npatch as is.\n\nBut I have this suspicion that this was a rather sloppy defensive check to\nprotect this codepath against potential breakage in another codepath (most\nlikely update_pre_post_images() touched by the same patch) that adjusts\nimage->line[].len the caller of this function uses to feed these two\nparameters. Giuseppe may have been not confident enough that the code\nadded to that function ensures not to undershoot when it reduces \"len\", or\nsomething.\n\nGiuseppe, can you explain what is going on?\n\nThanks\n\n> builtin/apply.c | 3 ---\n> 1 files changed, 0 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 84a8a0b..b3b59db 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -250,9 +250,6 @@ static int fuzzy_matchlines(const char *s1, size_t n1,\n> \tconst char *last2 = s2 + n2 - 1;\n> \tint result = 0;\n> \n> -\tif (n1 < 0 || n2 < 0)\n> -\t\treturn 0;\n> -\n> \t/* ignore line endings */\n> \twhile ((*last1 == '\\r') || (*last1 == '\\n'))\n> \t\tlast1--;\n"},{"id":"179031","messageId":"CACBZZX6CRm1W5i43=LeXPJFdcWFgVTkD8cGntHoKsPoWGx_hNg@mail.gmail.com","threadId":"28860","inReplyTo":"m2pqh5nvic.fsf@igel.home","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-11-07T12:42:39Z","receivedAt":"2011-11-07T12:42:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Nov 6, 2011 at 16:03, Andreas Schwab wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Remove an \"p->field < 0\" comparison in grep.c that'll always be\n>> false. In this case \"p\" is a \"grep_pat\" where \"field\" is defined as:\n>>\n>>       enum grep_header_field field;\n>>\n>> And grep_header_field is in turn defined as:\n>>\n>>     enum grep_header_field {\n>>       GREP_HEADER_AUTHOR = 0,\n>>       GREP_HEADER_COMMITTER\n>>     };\n>>\n>> Meaning that this comparison will always be false.\n>\n> The underlying integer type is implementation-defined, and can be any\n> signed or unsigned integer type that can represent all enumerations.\n\nYes, but as far as I can tell since we've done \"= 0\" there that\ndoesn't apply to us. To quote the ANSI C Standard (ANSI X3J11/88-090):\n\n 3.5.2.2 Enumeration specifiers\n\n Syntax\n\n enum-specifier:\n enum identifier { enumerator-list }\n enum identifier\n\n enumerator-list:\n enumerator\n enumerator-list , enumerator\n\n enumerator:\n enumeration-constant\n enumeration-constant = constant-expression\n\n Constraints\n\n The expression that defines the value of an enumeration constant\n shall be an integral constant expression that has a value\n representable as an int.\n\n Semantics\n\n The identifiers in an enumerator list are declared as constants\n that have type int and may appear wherever such are permitted./52/ An\n enumerator with = defines its enumeration constant as the value of the\n constant expression. If the first enumerator has no = , the value of\n its enumeration constant is 0. Each subsequent enumerator with no =\n defines its enumeration constant as the value of the constant\n expression obtained by adding 1 to the value of the previous\n enumeration constant. (A combination of both forms of enumerators may\n produce enumeration constants with values that duplicate other values\n in the same enumeration.) The enumerators of an enumeration are also\n known as its members.\n\n Each enumerated type shall be compatible with an integer type; the\n choice of type is implementation-defined.\n\n Example\n\n enum hue { chartreuse, burgundy, claret=20, winedark };\n /*...*/\n enum hue col, *cp;\n /*...*/\n col = claret;\n cp = &col;\n /*...*/\n /*...*/ (*cp != burgundy) /*...*/\n\n makes hue the tag of an enumeration, and then declares col as an\n object that has that type and cp as a pointer to an object that has\n that type. The enumerated values are in the set {0, 1, 20, 21}.\n\nI.e. we'll always have GREP_HEADER_AUTHOR = 0 and\nGREP_HEADER_COMMITTER = 1, we'll never have GREP_HEADER_AUTHOR = and\nGREP_HEADER_COMMITTER = .\n"},{"id":"179033","messageId":"m3pqh4krer.fsf@hase.home","threadId":"28860","inReplyTo":"CACBZZX6CRm1W5i43=LeXPJFdcWFgVTkD8cGntHoKsPoWGx_hNg@mail.gmail.com","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-11-07T13:12:28Z","receivedAt":"2011-11-07T13:12:28Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I.e. we'll always have GREP_HEADER_AUTHOR = 0 and\n> GREP_HEADER_COMMITTER = 1, we'll never have GREP_HEADER_AUTHOR = and\n> GREP_HEADER_COMMITTER = .\n\nThat is irrelevant. You can always assign -1 to an object of enumerated\ntype and the implicit conversion to the underlying type is fully\ndefined, no matter what type the compiler choses.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"179044","messageId":"20111107163823.GB27055@sigill.intra.peff.net","threadId":"28860","inReplyTo":"m3pqh4krer.fsf@hase.home","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-07T16:38:23Z","receivedAt":"2011-11-07T16:38:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 07, 2011 at 02:12:28PM +0100, Andreas Schwab wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > I.e. we'll always have GREP_HEADER_AUTHOR = 0 and\n> > GREP_HEADER_COMMITTER = 1, we'll never have GREP_HEADER_AUTHOR = and\n> > GREP_HEADER_COMMITTER = .\n> \n> That is irrelevant. You can always assign -1 to an object of enumerated\n> type and the implicit conversion to the underlying type is fully\n> defined, no matter what type the compiler choses.\n\nYes, but now you are getting into implementation-defined behavior, which\nis also something to avoid. IOW, I don't think it's wrong for a static\nanalyzer to complain about:\n\n if (enum_with_only_positive_values < 0)\n\njust because you could say:\n\n enum_with_only_positive_values = -1;\n\nand it would work on _some_ platforms. That same static analyzer should\nbe complaining about the second line, which is where the real potential\nbug is. If you want \"-1\" as an enumerated value, then add it to your\nenum definition and the compiler will do the right thing.\n\nIt is perfectly fine to do:\n\n enum foo { a = -1, b, c };\n ...\n if (foo < 0)\n ...\n\nand the static analyzer should not complain about the conditional there.\n\n-Peff\n"},{"id":"179057","messageId":"m2k47b4wqi.fsf@igel.home","threadId":"28860","inReplyTo":"20111107163823.GB27055@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-11-07T18:24:05Z","receivedAt":"2011-11-07T18:24:05Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Jeff King writes:\n\n> Yes, but now you are getting into implementation-defined behavior, which\n> is also something to avoid.\n\nThe whole point of the statement is a sanity check to uncover bugs. If\nyou remove the first condition you completely ruin its point.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"179059","messageId":"20111107183402.GA5118@sigill.intra.peff.net","threadId":"28860","inReplyTo":"m2k47b4wqi.fsf@igel.home","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-07T18:34:02Z","receivedAt":"2011-11-07T18:34:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 07, 2011 at 07:24:05PM +0100, Andreas Schwab wrote:\n\n> Jeff King writes:\n> \n> > Yes, but now you are getting into implementation-defined behavior, which\n> > is also something to avoid.\n> \n> The whole point of the statement is a sanity check to uncover bugs. If\n> you remove the first condition you completely ruin its point.\n\nI'm somewhat dubious of the value of a bug-check that may or may not\nactually kick in depending on your compiler's choice of enum\nrepresentation, and whose bugs are generally easier to check via static\nanalysis (i.e., making sure the enum value is one of the enumerated\nvalues when it is initialized or assigned).\n\nYes, static analysis can miss some bugs (like passing the address of the\nenum through a void pointer (e.g., when memset'ing a struct)). But\ncouldn't it just as easily be out of range in the other direction?\n\nIt seems like the bug trying to be caught is probably something like:\n\n enum foo v = function_which_returns_value_or_negative_error();\n if (v < 0)\n die(\"BUG\");\n\nBut in that case the bug is on the first line, and it is easily caught\nby static analysis, no?\n\n-Peff\n"},{"id":"179064","messageId":"20111107185536.GA5450@sigill.intra.peff.net","threadId":"28860","inReplyTo":"20111107183402.GA5118@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-07T18:55:36Z","receivedAt":"2011-11-07T18:55:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 07, 2011 at 01:34:02PM -0500, Jeff King wrote:\n\n> > The whole point of the statement is a sanity check to uncover bugs. If\n> > you remove the first condition you completely ruin its point.\n> [...]\n> Yes, static analysis can miss some bugs (like passing the address of the\n> enum through a void pointer (e.g., when memset'ing a struct)). But\n> couldn't it just as easily be out of range in the other direction?\n\nOops, I just looked at the actual conditional, and it is indeed\nrange-checking the whole enum. So if you are going to do that at all,\nthen yes, checking both sides is sensible, and that is what the original\nconditional does. Sorry for my confusion.\n\nI do still question the value of the check at all, though, given that\nstatic analysis can find those bugs.\n\n-Peff\n"},{"id":"179065","messageId":"CACBZZX7dipuDjCbb6oaLGHGYN-frgmRuLiDAebqs-LpQrJ=r2A@mail.gmail.com","threadId":"28860","inReplyTo":"20111107185536.GA5450@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2011-11-07T19:06:05Z","receivedAt":"2011-11-07T19:06:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Nov 7, 2011 at 19:55, Jeff King wrote:\n\n> I do still question the value of the check at all, though, given that\n> static analysis can find those bugs.\n\nAgreed, which is why I submitted this patch.\n\nAlso if this is the sort of thing we'd like to guard against we should\nbe discussing it more generally. We have a bunch of occurances where\nwe'd probably break down if someone manually assigned -1 to an enum\nvariable:\n\n $ git grep -E -A1 'enum.*\\{' | grep -B1 '=.*0' | grep enum\n builtin/branch.c:enum color_branch {\n builtin/branch.c:static enum merge_filter {\n builtin/fetch-pack.c:enum ack_type {\n builtin/fetch.c:enum {\n builtin/grep.c: enum {\n builtin/mv.c: enum update_mode { BOTH = 0, WORKING_DIRECTORY,\nINDEX } *modes;\n builtin/remote.c:enum {\n builtin/remote.c: enum {\n cache.h:enum rebase_setup_type {\n cache.h:enum push_default_type {\n cache.h:enum object_creation_mode {\n cache.h:enum sharedrepo {\n cache.h:enum date_mode {\n cache.h: enum {\n convert.h:enum safe_crlf {\n convert.h:enum auto_crlf {\n diff.h:enum diff_words_type {\n diff.h:enum color_diff {\n dir.c:enum exist_status {\n dir.h: enum {\n grep.h:enum grep_header_field {\n http-push.c:enum dav_header_flag {\n imap-send.c:enum CAPABILITY {\n log-tree.c:enum decoration_type {\n merge-recursive.c:enum rename_type {\n merge-recursive.h: enum {\n notes-merge.h: enum {\n remote.h:enum match_refs_flags {\n rerere.c: enum {\n unpack-trees.h:enum unpack_trees_error_types {\n wt-status.h:enum color_wt_status {\n"},{"id":"179067","messageId":"CAOxFTcxyOK=0rCeZtwQ5DOWu=T-h0qFo155+an+mYNCgWeyP8Q@mail.gmail.com","threadId":"28860","inReplyTo":"7vsjm15cap.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] apply: get rid of useless x < 0 comparison on a size_t type","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2011-11-07T19:09:23Z","receivedAt":"2011-11-07T19:09:23Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sun, Nov 6, 2011 at 7:35 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason   writes:\n>>\n>> This check was originally added in v1.6.5-rc0~53^2 by Giuseppe Bilotta\n>> while adding an option to git-apply to ignore whitespace differences.\n>\n> I agree that these quantities can never be negative, so I'll apply the\n> patch as is.\n\nI agree too. In fact I spotted this some time ago when I started\ncompiling git with Clang but never found the time to look into it.\n\n> But I have this suspicion that this was a rather sloppy defensive check to\n> protect this codepath against potential breakage in another codepath (most\n> likely update_pre_post_images() touched by the same patch) that adjusts\n> image->line[].len the caller of this function uses to feed these two\n> parameters. Giuseppe may have been not confident enough that the code\n> added to that function ensures not to undershoot when it reduces \"len\", or\n> something.\n>\n> Giuseppe, can you explain what is going on?\n\nNo, I can't, so I guess the sloppy coding is the right motivation. I\nremember working this patch from a rather older submission that was\nnever followed-up to, so my guess is that I just forgot to clean it up\nproperly, and during review the focus was obviously on other aspects\nof the submission too. Also, having a look at the current caller of\nthe function, I don't see how the check would even be needed.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"179070","messageId":"20111107194912.GA12469@elie.hsd1.il.comcast.net","threadId":"28860","inReplyTo":"1320581184-4557-4-git-send-email-avarab@gmail.com","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-07T19:49:13Z","receivedAt":"2011-11-07T19:49:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -327,7 +327,7 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)\n> \tfor (p = opt->header_list; p; p = p->next) {\n> \t\tif (p->token != GREP_PATTERN_HEAD)\n> \t\t\tdie(\"bug: a non-header pattern in grep header list.\");\n> -\t\tif (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)\n> +\t\tif (GREP_HEADER_FIELD_MAX <= p->field)\n\nI imagine the following would be less controversial.\n\n-- >8 --\nFrom: Ævar Arnfjörð Bjarmason \nSubject: grep: get rid of bounds check on \"enum grep_header_field\" value\n\nThe grep_header_field enum is defined as:\n\n enum grep_header_field {\n \tGREP_HEADER_AUTHOR = 0,\n \tGREP_HEADER_COMMITTER\n };\n\nGit's grep code sanity-checks the value of p->field before using it in\norder to avoid reading and writing past the end of an array.\nUnfortunately that test trips up misguided static analyzers like\n\"gcc -Wtype-limits\" that do not recognize that C allows this enum to\nbe a signed integer type and an \"x < 0\" comparison really could fire\nif someone passed in an uninitialized value.\n\nLet's just drop the check. Luckily git always does initialize\np->field correctly, and if it were to stop doing so, then hopefully\nrunning the test suite with valgrind would catch it.\n\nNoticed with clang -Wtautological-compare.\n\n[jn: drop the GREP_HEADER_FIELD_MAX <= p->field check, too,\n for symmetry]\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Jonathan Nieder \n---\n grep.c | 2 --\n 1 files changed, 0 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex b29d09c7..424c46cd 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -327,8 +327,6 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)\n \tfor (p = opt->header_list; p; p = p->next) {\n \t\tif (p->token != GREP_PATTERN_HEAD)\n \t\t\tdie(\"bug: a non-header pattern in grep header list.\");\n-\t\tif (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)\n-\t\t\tdie(\"bug: unknown header field %d\", p->field);\n \t\tcompile_regexp(p, opt);\n \t}\n \n-- \n1.7.8.rc0\n"},{"id":"179071","messageId":"m2d3d34ro5.fsf@igel.home","threadId":"28860","inReplyTo":"20111107185536.GA5450@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-11-07T20:13:30Z","receivedAt":"2011-11-07T20:13:30Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Jeff King writes:\n\n> I do still question the value of the check at all, though, given that\n> static analysis can find those bugs.\n\nThen you should remove the whole statement.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"179077","messageId":"7vlirr1vi5.fsf@alter.siamese.dyndns.org","threadId":"28860","inReplyTo":"20111107194912.GA12469@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-07T21:18:58Z","receivedAt":"2011-11-07T21:18:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> [jn: drop the GREP_HEADER_FIELD_MAX <= p->field check, too,\n> for symmetry]\n\nUmm, why is this removal of defensive programming practice an improvement?\nThis part is mostly my code and because I know what I wrote is almost\nalways perfect, so I do not think there is any real harm done, but still...\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> Signed-off-by: Jonathan Nieder \n> ---\n> grep.c | 2 --\n> 1 files changed, 0 insertions(+), 2 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index b29d09c7..424c46cd 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -327,8 +327,6 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)\n> \tfor (p = opt->header_list; p; p = p->next) {\n> \t\tif (p->token != GREP_PATTERN_HEAD)\n> \t\t\tdie(\"bug: a non-header pattern in grep header list.\");\n> -\t\tif (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)\n> -\t\t\tdie(\"bug: unknown header field %d\", p->field);\n> \t\tcompile_regexp(p, opt);\n> \t}\n"},{"id":"179080","messageId":"20111107213219.GA13537@elie.hsd1.il.comcast.net","threadId":"28860","inReplyTo":"7vlirr1vi5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-07T21:32:19Z","receivedAt":"2011-11-07T21:32:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> [jn: drop the GREP_HEADER_FIELD_MAX <= p->field check, too,\n>> for symmetry]\n>\n> Umm, why is this removal of defensive programming practice an improvement?\n\nChecking \"p->filed < 0\" makes static analyzers complain. There's no\nclean way I know of[*] to get them to shut up and keep the check. The\nfundamental question is whether the -Wtype-limits check is worth it or\nnot:\n\n - on one hand, it catches real bugs, such as the mistaken check for\n negative size_t Ævar mentioned;\n\n - on the other hand, it trips for cases like this in which the type\n only happened to be unsigned on the build platform. I consider\n that a gcc bug (and apparently clang shares it) --- see\n .\n\nSo, the purpose of this patch was to work around this common bug in\nstatic analyzers.\n\nChecking \"GREP_HEADER_FIELD_MAX <= p->field\" without checking for\n\"p->field < 0\", like the original patch did, would be only checking\nhalf of what it intends to and not add any real guarantees. And\nmore importantly, it would be distracting to people like me and\nAndreas who would wonder \"why doesn't this check p->field < 0, too?\".\n\nI guess the commit message should have said\n\n\tgrep: remove a defensive check to work around common static analyzer bug\n\n> This part is mostly my code and because I know what I wrote is almost\n> always perfect, so I do not think there is any real harm done, but still...\n\nHeh.\n\n[*] There are plenty of cryptic, hackish ways possible, though. :)\n\n\tif ((size_t) p->field >= GREP_HEADER_FIELD_MAX)\n\t\tdie(...);\n"},{"id":"179087","messageId":"7vd3d31u4e.fsf@alter.siamese.dyndns.org","threadId":"28860","inReplyTo":"20111107213219.GA13537@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-07T21:48:49Z","receivedAt":"2011-11-07T21:48:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> So, the purpose of this patch was to work around this common bug in\n> static analyzers.\n\nI fail to see how it could be even considered a work around.\n\nIf you do not use static analyzers, you do not have to do such a change,\nand the resulting code would (not the \"negative\" side, but the \"positive\"\nside) catch real bugs when somebody screwes up and stuffs a bogus oob\nvalue in p->field.\n\nWith the removal of the check, you _have_ to rely on static analyzers to\ndo the _right thing_, but if you have static analyzers that do the right\nthing, you do not have to have such a workaround to begin with.\n"},{"id":"179091","messageId":"20111107222109.GA22693@elie.hsd1.il.comcast.net","threadId":"28860","inReplyTo":"7vd3d31u4e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] grep: get rid of useless x < 0 comparison on an enum member","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-07T22:21:09Z","receivedAt":"2011-11-07T22:21:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> With the removal of the check, you _have_ to rely on static analyzers to\n> do the _right thing_, but if you have static analyzers that do the right\n> thing, you do not have to have such a workaround to begin with.\n\nNo, automatic static analyzers will not help you avoid using an\nuninitialized enum value here (well, I guess they could, but I don't\nbelieve gcc or clang is nearly smart enough for that). The only\nreplacement for the check is (1) humans and (2) valgrind.\n\nIn this particular case, as you mentioned before, (1) seems to be\nquite sufficient.\n"},{"id":"179141","messageId":"CABPp-BFm-BYPuGzp4hpH2ub_GBLWcZMKt8=rOR683_-+HFbb5A@mail.gmail.com","threadId":"28860","inReplyTo":"CACBZZX7bzFy_NCr4RTpYXXVmieaV7GrBfJgwdtCT1SwoXV_cGA@mail.gmail.com","subject":"Re: [PATCH 0/3] Fix code issues spotted by clang","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2011-11-08T16:05:57Z","receivedAt":"2011-11-08T16:05:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Nov 6, 2011 at 5:33 AM, Ævar Arnfjörð Bjarmason\n wrote:\n>    \"builtin/fast-export.c\", line 54: warning: enum type mismatch: op \"=\"\n>\n> This seems to me to be a legitimate issue we introduced in\n> 2d8ad46919213ebbd7bb72eb5b56cca8cc3ae07f, Elijah?\n>\n> We're defining an enum like this:\n>\n>    static enum { ABORT, VERBATIM, WARN, STRIP } signed_tag_mode = ABORT;\n>    static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ABORT;\n>\n> And then doing:\n>\n>        if (unset || !strcmp(arg, \"abort\"))\n>                tag_of_filtered_mode = ABORT; <-- Line 54\n>        else if (!strcmp(arg, \"drop\"))\n>                tag_of_filtered_mode = DROP;\n>        else if (!strcmp(arg, \"rewrite\"))\n>                tag_of_filtered_mode = REWRITE;\n>\n> Presumably that assignment should be \"= ERROR\".\n\nLooking back at that patch, \"abort\" was the word that made the most\nsense in the documentation and I was trying to make the code match.\nUnfortunately, that would result in \"ABORT\" being redefined (to the\nsame value) so to squelch the compiler error I just went with the\nquick hack of changing \"ABORT\" in the enum definition to \"ERROR\", but\ndidn't change \"ABORT\" later since \"it had the same value anyway\".\nYeah, not the safest programming practice.\n\nI probably should have just used FILTERED_TAG_ABORT and changed the\nsigned_tag_mode enum to use SIGNED_TAG_ABORT (and then changed all\nreferences to \"ABORT\" to match one of those two)...or maybe somehow\ncome up with a combined enum used for both (though WARN would only be\nuseful fo signed_tag_mode, REWRITE would only be useful for\ntag_of_filtered_mode, etc.).\n"}]}