{"thread":{"id":"37608","subject":"[PATCH] mailinfo: resolve -Wstring-plus-int warning","startedAt":"2014-09-21T09:13:58Z","lastAt":"2014-09-23T08:12:25Z","messageCount":12,"participants":["Eric Sunshine","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"249717","messageId":"1411290838-45622-1-git-send-email-sunshine@sunshineco.com","threadId":"37608","inReplyTo":null,"subject":"[PATCH] mailinfo: resolve -Wstring-plus-int warning","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-09-21T09:13:58Z","receivedAt":"2014-09-21T09:13:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The just-released Apple Xcode 6.0.1 has -Wstring-plus-int enabled by\ndefault which complains about pointer arithmetic applied to a string\nliteral:\n\n builtin/mailinfo.c:303:24: warning:\n adding 'long' to a string does not append to the string\n return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) ...\n ~~~~~~~^~~~~~~~~~~~~\n\nResolve this issue.\n\nSigned-off-by: Eric Sunshine \n---\n\nThis is atop 2da1f366 (mailinfo: make \">From\" in-body header check more\nrobust; 2014-09-13) in 'next'.\n\nIn addition to the above diagnostic, the Apple compiler also helpfully\nrecommends &SAMPLE[cp - line] as a replacement to avoid the warning,\nhowever, the solution in this patch allows us drop a couple strlen()s in\nfavor of sizeof()s.\n\n builtin/mailinfo.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/mailinfo.c b/builtin/mailinfo.c\nindex 2632fb0..b6b1c19 100644\n--- a/builtin/mailinfo.c\n+++ b/builtin/mailinfo.c\n@@ -288,19 +288,20 @@ static inline int cmp_header(const struct strbuf *line, const char *hdr)\n \t\t\tline->buf[len] == ':' && isspace(line->buf[len + 1]);\n }\n \n-#define SAMPLE \"From e6807f3efca28b30decfecb1732a56c7db1137ee Mon Sep 17 00:00:00 2001\\n\"\n static int is_format_patch_separator(const char *line, int len)\n {\n+\tstatic const char SAMPLE[] =\n+\t\t\"From e6807f3efca28b30decfecb1732a56c7db1137ee Mon Sep 17 00:00:00 2001\\n\";\n \tconst char *cp;\n \n-\tif (len != strlen(SAMPLE))\n+\tif (len != sizeof(SAMPLE) - 1)\n \t\treturn 0;\n \tif (!skip_prefix(line, \"From \", &cp))\n \t\treturn 0;\n \tif (strspn(cp, \"0123456789abcdef\") != 40)\n \t\treturn 0;\n \tcp += 40;\n-\treturn !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line));\n+\treturn !memcmp(SAMPLE + (cp - line), cp, sizeof(SAMPLE) - 1 - (cp - line));\n }\n \n static int check_header(const struct strbuf *line,\n-- \n2.1.1.391.g7a54a76.dirty\n"},{"id":"249742","messageId":"xmqqk34vlfhz.fsf@gitster.dls.corp.google.com","threadId":"37608","inReplyTo":"1411290838-45622-1-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-22T17:41:28Z","receivedAt":"2014-09-22T17:41:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> The just-released Apple Xcode 6.0.1 has -Wstring-plus-int enabled by\n> default which complains about pointer arithmetic applied to a string\n> literal:\n>\n> builtin/mailinfo.c:303:24: warning:\n> adding 'long' to a string does not append to the string\n> return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) ...\n> ~~~~~~~^~~~~~~~~~~~~\n\nAnd why is that a warning-worthy violation? Can we have them fix\ntheir compiler instead?\n\n>\n> Resolve this issue.\n>\n> Signed-off-by: Eric Sunshine \n> ---\n>\n> This is atop 2da1f366 (mailinfo: make \">From\" in-body header check more\n> robust; 2014-09-13) in 'next'.\n>\n> In addition to the above diagnostic, the Apple compiler also helpfully\n> recommends &SAMPLE[cp - line] as a replacement to avoid the warning,\n> however, the solution in this patch allows us drop a couple strlen()s in\n> favor of sizeof()s.\n>\n> builtin/mailinfo.c | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c\n> index 2632fb0..b6b1c19 100644\n> --- a/builtin/mailinfo.c\n> +++ b/builtin/mailinfo.c\n> @@ -288,19 +288,20 @@ static inline int cmp_header(const struct strbuf *line, const char *hdr)\n> \t\t\tline->buf[len] == ':' && isspace(line->buf[len + 1]);\n> }\n> \n> -#define SAMPLE \"From e6807f3efca28b30decfecb1732a56c7db1137ee Mon Sep 17 00:00:00 2001\\n\"\n> static int is_format_patch_separator(const char *line, int len)\n> {\n> +\tstatic const char SAMPLE[] =\n> +\t\t\"From e6807f3efca28b30decfecb1732a56c7db1137ee Mon Sep 17 00:00:00 2001\\n\";\n> \tconst char *cp;\n> \n> -\tif (len != strlen(SAMPLE))\n> +\tif (len != sizeof(SAMPLE) - 1)\n> \t\treturn 0;\n> \tif (!skip_prefix(line, \"From \", &cp))\n> \t\treturn 0;\n> \tif (strspn(cp, \"0123456789abcdef\") != 40)\n> \t\treturn 0;\n> \tcp += 40;\n> -\treturn !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line));\n> +\treturn !memcmp(SAMPLE + (cp - line), cp, sizeof(SAMPLE) - 1 - (cp - line));\n> }\n> \n> static int check_header(const struct strbuf *line,\n"},{"id":"249751","messageId":"xmqqoau7js6a.fsf@gitster.dls.corp.google.com","threadId":"37608","inReplyTo":"1411290838-45622-1-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-22T20:50:37Z","receivedAt":"2014-09-22T20:50:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> however, the solution in this patch allows us drop a couple strlen()s in\n> favor of sizeof()s.\n\nIt is actually not a very good justification when you know you care\nabout the length of the string. A decent compiler ought to know the\nlength of a constant string can be computed at the compilation time.\n\nLet's at least not do that part of the change.\n"},{"id":"249755","messageId":"CAPig+cTAFaG5H8rmf1jrvFwr_OOH7u19JMKDUE12UddokUmfaQ@mail.gmail.com","threadId":"37608","inReplyTo":"xmqqk34vlfhz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-09-22T21:10:08Z","receivedAt":"2014-09-22T21:10:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 22, 2014 at 1:41 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>> The just-released Apple Xcode 6.0.1 has -Wstring-plus-int enabled by\n>> default which complains about pointer arithmetic applied to a string\n>> literal:\n>>\n>> builtin/mailinfo.c:303:24: warning:\n>> adding 'long' to a string does not append to the string\n>> return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) ...\n>> ~~~~~~~^~~~~~~~~~~~~\n>\n> And why is that a warning-worthy violation?\n\nNot being privy to Apple's decision making process, I can only guess\nthat it is in response to their new Swift programming language which\nthey are pushing heavily on iOS (and soon Mac OS X), in which '+' is\nthe string concatenation operator. For projects written in Swift and\nincorporating legacy or portable components in C, C++, or Objective-C,\nthe warning may help programmer's avoid the pitfall of thinking that\n'+' is also concatenation in the C-based languages.\n\n> Can we have them fix their compiler instead?\n\nIf the above supposition is correct, then it's likely that Apple\nconsiders this a feature, not a bug which needs to be fixed.\n\n>> Resolve this issue.\n>>\n>> Signed-off-by: Eric Sunshine \n"},{"id":"249758","messageId":"CAPig+cS9oa+9U3RQgaQ6iTxO0QBOATSU4b426fwcpS2GFJWDpQ@mail.gmail.com","threadId":"37608","inReplyTo":"xmqqoau7js6a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-09-22T21:50:01Z","receivedAt":"2014-09-22T21:50:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 22, 2014 at 4:50 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>> however, the solution in this patch allows us drop a couple strlen()s in\n>> favor of sizeof()s.\n>\n> It is actually not a very good justification when you know you care\n> about the length of the string. A decent compiler ought to know the\n> length of a constant string can be computed at the compilation time.\n\nIndeed, which is why I mentioned this only in the commentary rather\nthan the commit message; but I was also thinking of some of the\nless-than-decent compilers with which git is sometimes built.\n\n> Let's at least not do that part of the change.\n\nI don't have strong feelings about it.\n"},{"id":"249764","messageId":"20140923060407.GA23861@peff.net","threadId":"37608","inReplyTo":"CAPig+cTAFaG5H8rmf1jrvFwr_OOH7u19JMKDUE12UddokUmfaQ@mail.gmail.com","subject":"Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-23T06:04:07Z","receivedAt":"2014-09-23T06:04:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 22, 2014 at 05:10:08PM -0400, Eric Sunshine wrote:\n\n> On Mon, Sep 22, 2014 at 1:41 PM, Junio C Hamano wrote:\n> > Eric Sunshine writes:\n> >\n> >> The just-released Apple Xcode 6.0.1 has -Wstring-plus-int enabled by\n> >> default which complains about pointer arithmetic applied to a string\n> >> literal:\n> >>\n> >> builtin/mailinfo.c:303:24: warning:\n> >> adding 'long' to a string does not append to the string\n> >> return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) ...\n> >> ~~~~~~~^~~~~~~~~~~~~\n> >\n> > And why is that a warning-worthy violation?\n> \n> Not being privy to Apple's decision making process, I can only guess\n> that it is in response to their new Swift programming language which\n> they are pushing heavily on iOS (and soon Mac OS X), in which '+' is\n> the string concatenation operator. For projects written in Swift and\n> incorporating legacy or portable components in C, C++, or Objective-C,\n> the warning may help programmer's avoid the pitfall of thinking that\n> '+' is also concatenation in the C-based languages.\n\nThat is my reading from the warning text, too, but I have to wonder:\nwouldn't that mean they should be warning about pointer + pointer, not\npointer + int?\n\nAlso, wouldn't the same advice apply to adding to _any_ char pointer,\nnot just a string literal?\n\nI know you don't have answers to those questions, but the whole thing\nseems rather silly to me.\n\n> > Can we have them fix their compiler instead?\n> \n> If the above supposition is correct, then it's likely that Apple\n> considers this a feature, not a bug which needs to be fixed.\n\nI don't mind silencing this one warning (even though I find it a little\nridiculous). I'm slightly concerned that more brain-damage may be coming\nour way, but we can deal with that if it ever does.\n\nLike Junio, I prefer keeping strlen() rather than switching to sizeof,\nas it is less error-prone (no need for extra \"-1\" dance, and it won't\nsilently do the wrong thing if the array is ever converted to a\npointer).\n\n-Peff\n"},{"id":"249765","messageId":"CAPc5daUbXtNXSn8_tspvdF+SH5aeX+jVJTramtOm96Dc1wCqtg@mail.gmail.com","threadId":"37608","inReplyTo":"20140923060407.GA23861@peff.net","subject":"Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-23T06:26:14Z","receivedAt":"2014-09-23T06:26:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Mon, Sep 22, 2014 at 11:04 PM, Jeff King wrote:\n>\n> I don't mind silencing this one warning (even though I find it a little\n> ridiculous). I'm slightly concerned that more brain-damage may be coming\n> our way, but we can deal with that if it ever does.\n>\n> Like Junio, I prefer keeping strlen() rather than switching to sizeof,\n> as it is less error-prone (no need for extra \"-1\" dance, and it won't\n> silently do the wrong thing if the array is ever converted to a\n> pointer).\n\nI actually do not mind losing the sample[] array too much.\n\nThe early 45 bytes or so of that array (or a string constant) is not used\nby the code at all; I didn't want to count \"From \" (that's 5), 40-hex and\nthen a SP -- ah, see, it is 46 bytes and I didn't want such miscounting.\nThe only real contents that matter in that sample[] array is the tail part\nthat is meant as the magic(5) cue. I'd be OK if the code checked the\nlength of the line against a hardcoded constant and then did strcmp()\nstarting from a hardcoded offset of the string and the magic cue string,\nand that would also avoid the warning from Eric's compiler.\n\nBut personally, I think the way it is coded is much easier to read,\nand is much harder to get it wrong while maintaining it. So...\n"},{"id":"249769","messageId":"20140923075146.GA11104@peff.net","threadId":"37608","inReplyTo":"CAPc5daUbXtNXSn8_tspvdF+SH5aeX+jVJTramtOm96Dc1wCqtg@mail.gmail.com","subject":"Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-23T07:51:46Z","receivedAt":"2014-09-23T07:51:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 22, 2014 at 11:26:14PM -0700, Junio C Hamano wrote:\n\n> On Mon, Sep 22, 2014 at 11:04 PM, Jeff King wrote:\n> >\n> > I don't mind silencing this one warning (even though I find it a little\n> > ridiculous). I'm slightly concerned that more brain-damage may be coming\n> > our way, but we can deal with that if it ever does.\n> >\n> > Like Junio, I prefer keeping strlen() rather than switching to sizeof,\n> > as it is less error-prone (no need for extra \"-1\" dance, and it won't\n> > silently do the wrong thing if the array is ever converted to a\n> > pointer).\n> \n> I actually do not mind losing the sample[] array too much.\n> \n> The early 45 bytes or so of that array (or a string constant) is not used\n> by the code at all; I didn't want to count \"From \" (that's 5), 40-hex and\n> then a SP -- ah, see, it is 46 bytes and I didn't want such miscounting.\n> The only real contents that matter in that sample[] array is the tail part\n> that is meant as the magic(5) cue. I'd be OK if the code checked the\n> length of the line against a hardcoded constant and then did strcmp()\n> starting from a hardcoded offset of the string and the magic cue string,\n> and that would also avoid the warning from Eric's compiler.\n> \n> But personally, I think the way it is coded is much easier to read,\n> and is much harder to get it wrong while maintaining it. So...\n\nI agree. I was going to suggest switching to a static const array\ninstead of a string literal, but retaining strlen()...but I see you\nalready queued that in pu. So if what is there works for Eric (I do not\nhave the compiler in question to test with), that seems reasonable.\n\n-Peff\n"},{"id":"249770","messageId":"CAPig+cQmbOs7Xw8wv63mLHfpG13Vo+tR7oLq-5srCcP1QQddnQ@mail.gmail.com","threadId":"37608","inReplyTo":"20140923060407.GA23861@peff.net","subject":"Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-09-23T07:52:21Z","receivedAt":"2014-09-23T07:52:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 23, 2014 at 2:04 AM, Jeff King wrote:\n> On Mon, Sep 22, 2014 at 05:10:08PM -0400, Eric Sunshine wrote:\n>\n>> On Mon, Sep 22, 2014 at 1:41 PM, Junio C Hamano wrote:\n>> > Eric Sunshine writes:\n>> >\n>> >> The just-released Apple Xcode 6.0.1 has -Wstring-plus-int enabled by\n>> >> default which complains about pointer arithmetic applied to a string\n>> >> literal:\n>> >>\n>> >> builtin/mailinfo.c:303:24: warning:\n>> >> adding 'long' to a string does not append to the string\n>> >> return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) ...\n>> >> ~~~~~~~^~~~~~~~~~~~~\n>> >\n>> > And why is that a warning-worthy violation?\n>>\n>> Not being privy to Apple's decision making process, I can only guess\n>> that it is in response to their new Swift programming language which\n>> they are pushing heavily on iOS (and soon Mac OS X), in which '+' is\n>> the string concatenation operator. For projects written in Swift and\n>> incorporating legacy or portable components in C, C++, or Objective-C,\n>> the warning may help programmer's avoid the pitfall of thinking that\n>> '+' is also concatenation in the C-based languages.\n>\n> That is my reading from the warning text, too, but I have to wonder:\n> wouldn't that mean they should be warning about pointer + pointer, not\n> pointer + int?\n\n'pointer + pointer' is not legal C, is it? What would the result\nrepresent? The compiler correctly diagnoses that case as an error\n(without special command-line switches):\n\n error: invalid operands to binary expression\n ('char *' and 'char *')\n return \"a\" + \"b\";\n ~~~ ^ ~~~\n\n> Also, wouldn't the same advice apply to adding to _any_ char pointer,\n> not just a string literal?\n\nNot really. Indexing into a char array via pointer arithmetic is a\nperfectly reasonable and common idiom in C (indeed, git is peppered\nwith it), so such a warning would be pure noise. The more restricted\ncase of 'stringliteral + something' is probably sufficiently rare that\ndiagnosing it may catch genuine errors by programmers coming from\nlanguages in which '+' does string concatenation (which may be the\noriginal motivation for the warning; see below).\n\n> I know you don't have answers to those questions, but the whole thing\n> seems rather silly to me.\n\nI did some more research and discovered that -Wstring-plus-int was\nadded in response to 'string + int' cases being found in Chromium's C\ncode (perhaps by programmers living in both JavaScript and C worlds).\nThe proposed option was discussed [1] and committed [2]. Later a\n-Wstring-plus-char option was also discussed [3] and committed [4].\n\nRegarding Apple: The Swift language supports 'string + string' and\n'string + char', but not 'string + int'. However, in C, the char would\nbe promoted to int, so there is some sense in warning about 'string +\nint' if they want to help safeguard programmers coming to C from Swift\n(but, again, I'm just speculating).\n\n[1]: http://thread.gmane.org/gmane.comp.compilers.clang.scm/47203/\n[2]: http://llvm.org/klaus/clang/commit/1cb2d742eb6635aeab6132ee5f0b5781d39487d7/\n[3]: http://thread.gmane.org/gmane.comp.compilers.clang.scm/82462/\n[4]: http://llvm.org/klaus/clang/commit/02debf605cd904edac8dceb196e5f142ce3d14eb/\n\n>> > Can we have them fix their compiler instead?\n>>\n>> If the above supposition is correct, then it's likely that Apple\n>> considers this a feature, not a bug which needs to be fixed.\n>\n> Like Junio, I prefer keeping strlen() rather than switching to sizeof,\n> as it is less error-prone (no need for extra \"-1\" dance, and it won't\n> silently do the wrong thing if the array is ever converted to a\n> pointer).\n\nSounds reasonable.\n"},{"id":"249771","messageId":"CAPig+cRZ++QF0az2+1H1QqECqPVdK-99T=R7EN02FfReV7C6UA@mail.gmail.com","threadId":"37608","inReplyTo":"CAPc5daUbXtNXSn8_tspvdF+SH5aeX+jVJTramtOm96Dc1wCqtg@mail.gmail.com","subject":"Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-09-23T07:58:11Z","receivedAt":"2014-09-23T07:58:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 23, 2014 at 2:26 AM, Junio C Hamano wrote:\n> On Mon, Sep 22, 2014 at 11:04 PM, Jeff King wrote:\n>>\n>> I don't mind silencing this one warning (even though I find it a little\n>> ridiculous). I'm slightly concerned that more brain-damage may be coming\n>> our way, but we can deal with that if it ever does.\n>>\n>> Like Junio, I prefer keeping strlen() rather than switching to sizeof,\n>> as it is less error-prone (no need for extra \"-1\" dance, and it won't\n>> silently do the wrong thing if the array is ever converted to a\n>> pointer).\n>\n> I actually do not mind losing the sample[] array too much.\n>\n> The early 45 bytes or so of that array (or a string constant) is not used\n> by the code at all; I didn't want to count \"From \" (that's 5), 40-hex and\n> then a SP -- ah, see, it is 46 bytes and I didn't want such miscounting.\n> The only real contents that matter in that sample[] array is the tail part\n> that is meant as the magic(5) cue. I'd be OK if the code checked the\n> length of the line against a hardcoded constant and then did strcmp()\n> starting from a hardcoded offset of the string and the magic cue string,\n> and that would also avoid the warning from Eric's compiler.\n\nThe -Wstring-plus-int option is \"smart\" enough to suppress the warning\nif the hardcoded offset falls within the bounds of the string literal,\nso this could work (but it feels a bit fragile compared to the current\ncode).\n\n> But personally, I think the way it is coded is much easier to read,\n> and is much harder to get it wrong while maintaining it. So...\n"},{"id":"249773","messageId":"CAPig+cQE2hKU0Q+8nMRyC3V1YgKVJDkyJCAR-gBh0UoawFAoUA@mail.gmail.com","threadId":"37608","inReplyTo":"20140923075146.GA11104@peff.net","subject":"Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-09-23T08:05:51Z","receivedAt":"2014-09-23T08:05:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 23, 2014 at 3:51 AM, Jeff King wrote:\n> On Mon, Sep 22, 2014 at 11:26:14PM -0700, Junio C Hamano wrote:\n>\n>> On Mon, Sep 22, 2014 at 11:04 PM, Jeff King wrote:\n>> >\n>> > I don't mind silencing this one warning (even though I find it a little\n>> > ridiculous). I'm slightly concerned that more brain-damage may be coming\n>> > our way, but we can deal with that if it ever does.\n>> >\n>> > Like Junio, I prefer keeping strlen() rather than switching to sizeof,\n>> > as it is less error-prone (no need for extra \"-1\" dance, and it won't\n>> > silently do the wrong thing if the array is ever converted to a\n>> > pointer).\n>>\n>> I actually do not mind losing the sample[] array too much.\n>>\n>> But personally, I think the way it is coded is much easier to read,\n>> and is much harder to get it wrong while maintaining it. So...\n>\n> I agree. I was going to suggest switching to a static const array\n> instead of a string literal, but retaining strlen()...but I see you\n> already queued that in pu. So if what is there works for Eric (I do not\n> have the compiler in question to test with), that seems reasonable.\n\nWhat is queued in 'pu' is the same as my patch [1] minus the\nsuperfluous strlen() => sizeof() change, so it works fine for me.\nThanks.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/257345\n"},{"id":"249775","messageId":"20140923081225.GB11104@peff.net","threadId":"37608","inReplyTo":"CAPig+cQmbOs7Xw8wv63mLHfpG13Vo+tR7oLq-5srCcP1QQddnQ@mail.gmail.com","subject":"Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-23T08:12:25Z","receivedAt":"2014-09-23T08:12:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 23, 2014 at 03:52:21AM -0400, Eric Sunshine wrote:\n\n> > That is my reading from the warning text, too, but I have to wonder:\n> > wouldn't that mean they should be warning about pointer + pointer, not\n> > pointer + int?\n> \n> 'pointer + pointer' is not legal C, is it? What would the result\n> represent? The compiler correctly diagnoses that case as an error\n> (without special command-line switches):\n> \n> error: invalid operands to binary expression\n> ('char *' and 'char *')\n> return \"a\" + \"b\";\n> ~~~ ^ ~~~\n\nYou're correct that it's not legal. My point was more that \"pointer +\nint\" is already clearly not string concatenation, because the operands\nare not two strings.\n\nI think the answer here (from the threads you linked below) is that\npeople expect it to not just concatenate, but also auto-convert integers\nto strings. Yeesh.\n\n> > Also, wouldn't the same advice apply to adding to _any_ char pointer,\n> > not just a string literal?\n> \n> Not really. Indexing into a char array via pointer arithmetic is a\n> perfectly reasonable and common idiom in C (indeed, git is peppered\n> with it), so such a warning would be pure noise.\n\nThat is a good reason not to do the warning in these cases (and why I\nhope that we will not have to deal with this further). But IMHO it is\ngood evidence that the warning is not well thought-out. It seems silly\nthat:\n\n x = \"foo\" + 1;\n\nis bad, but:\n\n y = \"foo\";\n x = y + 1;\n\nis not[1]. Saying the first one is rare does not seem like a good\nexcuse; rather the existence of the second should tip you off that the\nidiom is valid and not to be complained about.\n\nAnyway, there is not much point in me complaining further about it here.\nYou are not the one who introduced it. :)\n\nThanks for digging in the history. It was interesting at least.\n\n-Peff\n\n[1] Actually, from reading the patch thread, I think the \"1\" needs to be\n a non-constant integer here. But that just furthers the point: if\n you have to neuter the warning to prevent a ton of false positives,\n that is a good indication that you should not be warning. :)\n"}]}