threads / patch / 37608

patchmailinfo: resolve -Wstring-plus-int warning

Subject: [PATCH] mailinfo: resolve -Wstring-plus-int warning

## tl;dr

12 messages between Sep 21, 2014 and Sep 23, 2014. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Eric Sunshine· Sep 21, 2014, 09:13 UTC · lore

The just-released Apple Xcode 6.0.1 has -Wstring-plus-int enabled by default which complains about pointer arithmetic applied to a string literal:

    builtin/mailinfo.c:303:24: warning:
        adding 'long' to a string does not append to the string
            return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) ...
                           ~~~~~~~^~~~~~~~~~~~~
Resolve this issue.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

This is atop 2da1f366 (mailinfo: make ">From" in-body header check more robust; 2014-09-13) in 'next'.

In addition to the above diagnostic, the Apple compiler also helpfully recommends &SAMPLE[cp - line] as a replacement to avoid the warning, however, the solution in this patch allows us drop a couple strlen()s in favor of sizeof()s.

 builtin/mailinfo.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Show changes to builtin/mailinfo.c +4 −3
diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index 2632fb0..b6b1c19 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -288,19 +288,20 @@ static inline int cmp_header(const struct strbuf *line, const char *hdr)
 			line->buf[len] == ':' && isspace(line->buf[len + 1]);
 }
 
-#define SAMPLE "From e6807f3efca28b30decfecb1732a56c7db1137ee Mon Sep 17 00:00:00 2001\n"
 static int is_format_patch_separator(const char *line, int len)
 {
+	static const char SAMPLE[] =
+		"From e6807f3efca28b30decfecb1732a56c7db1137ee Mon Sep 17 00:00:00 2001\n";
 	const char *cp;
 
-	if (len != strlen(SAMPLE))
+	if (len != sizeof(SAMPLE) - 1)
 		return 0;
 	if (!skip_prefix(line, "From ", &cp))
 		return 0;
 	if (strspn(cp, "0123456789abcdef") != 40)
 		return 0;
 	cp += 40;
-	return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line));
+	return !memcmp(SAMPLE + (cp - line), cp, sizeof(SAMPLE) - 1 - (cp - line));
 }
 
 static int check_header(const struct strbuf *line,

-- 
2.1.1.391.g7a54a76.dirty
Junio C Hamano· Sep 22, 2014, 17:41 UTC · re: Eric Sunshine · lore

Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 8 quoted lines
> The just-released Apple Xcode 6.0.1 has -Wstring-plus-int enabled by
> default which complains about pointer arithmetic applied to a string
> literal:
>
>     builtin/mailinfo.c:303:24: warning:
>         adding 'long' to a string does not append to the string
>             return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) ...
>                            ~~~~~~~^~~~~~~~~~~~~

And why is that a warning-worthy violation? Can we have them fix their compiler instead?

Show 45 quoted lines
>
> Resolve this issue.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>
> This is atop 2da1f366 (mailinfo: make ">From" in-body header check more
> robust; 2014-09-13) in 'next'.
>
> In addition to the above diagnostic, the Apple compiler also helpfully
> recommends &SAMPLE[cp - line] as a replacement to avoid the warning,
> however, the solution in this patch allows us drop a couple strlen()s in
> favor of sizeof()s.
>
>  builtin/mailinfo.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
> index 2632fb0..b6b1c19 100644
> --- a/builtin/mailinfo.c
> +++ b/builtin/mailinfo.c
> @@ -288,19 +288,20 @@ static inline int cmp_header(const struct strbuf *line, const char *hdr)
>  			line->buf[len] == ':' && isspace(line->buf[len + 1]);
>  }
>  
> -#define SAMPLE "From e6807f3efca28b30decfecb1732a56c7db1137ee Mon Sep 17 00:00:00 2001\n"
>  static int is_format_patch_separator(const char *line, int len)
>  {
> +	static const char SAMPLE[] =
> +		"From e6807f3efca28b30decfecb1732a56c7db1137ee Mon Sep 17 00:00:00 2001\n";
>  	const char *cp;
>  
> -	if (len != strlen(SAMPLE))
> +	if (len != sizeof(SAMPLE) - 1)
>  		return 0;
>  	if (!skip_prefix(line, "From ", &cp))
>  		return 0;
>  	if (strspn(cp, "0123456789abcdef") != 40)
>  		return 0;
>  	cp += 40;
> -	return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line));
> +	return !memcmp(SAMPLE + (cp - line), cp, sizeof(SAMPLE) - 1 - (cp - line));
>  }
>  
>  static int check_header(const struct strbuf *line,
Eric Sunshine· Sep 22, 2014, 21:10 UTC · re: Junio C Hamano · lore

Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning

On Mon, Sep 22, 2014 at 1:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> The just-released Apple Xcode 6.0.1 has -Wstring-plus-int enabled by
>> default which complains about pointer arithmetic applied to a string
>> literal:
>>
>>     builtin/mailinfo.c:303:24: warning:
>>         adding 'long' to a string does not append to the string
>>             return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) ...
>>                            ~~~~~~~^~~~~~~~~~~~~
>
> And why is that a warning-worthy violation?

Not being privy to Apple's decision making process, I can only guess that it is in response to their new Swift programming language which they are pushing heavily on iOS (and soon Mac OS X), in which '+' is the string concatenation operator. For projects written in Swift and incorporating legacy or portable components in C, C++, or Objective-C, the warning may help programmer's avoid the pitfall of thinking that '+' is also concatenation in the C-based languages.

> Can we have them fix their compiler instead?

If the above supposition is correct, then it's likely that Apple considers this a feature, not a bug which needs to be fixed.

>> Resolve this issue.
>>
>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Jeff King· Sep 23, 2014, 06:04 UTC · re: Eric Sunshine · lore

Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning

On Mon, Sep 22, 2014 at 05:10:08PM -0400, Eric Sunshine wrote:
Show 21 quoted lines
> On Mon, Sep 22, 2014 at 1:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> >
> >> The just-released Apple Xcode 6.0.1 has -Wstring-plus-int enabled by
> >> default which complains about pointer arithmetic applied to a string
> >> literal:
> >>
> >>     builtin/mailinfo.c:303:24: warning:
> >>         adding 'long' to a string does not append to the string
> >>             return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) ...
> >>                            ~~~~~~~^~~~~~~~~~~~~
> >
> > And why is that a warning-worthy violation?
> 
> Not being privy to Apple's decision making process, I can only guess
> that it is in response to their new Swift programming language which
> they are pushing heavily on iOS (and soon Mac OS X), in which '+' is
> the string concatenation operator. For projects written in Swift and
> incorporating legacy or portable components in C, C++, or Objective-C,
> the warning may help programmer's avoid the pitfall of thinking that
> '+' is also concatenation in the C-based languages.

That is my reading from the warning text, too, but I have to wonder: wouldn't that mean they should be warning about pointer + pointer, not pointer + int?

Also, wouldn't the same advice apply to adding to _any_ char pointer, not just a string literal?

I know you don't have answers to those questions, but the whole thing seems rather silly to me.

> > Can we have them fix their compiler instead?
> 
> If the above supposition is correct, then it's likely that Apple
> considers this a feature, not a bug which needs to be fixed.

I don't mind silencing this one warning (even though I find it a little ridiculous). I'm slightly concerned that more brain-damage may be coming our way, but we can deal with that if it ever does.

Like Junio, I prefer keeping strlen() rather than switching to sizeof, as it is less error-prone (no need for extra "-1" dance, and it won't silently do the wrong thing if the array is ever converted to a pointer).

-Peff
Junio C Hamano· Sep 23, 2014, 06:26 UTC · re: Jeff King · lore

Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning

On Mon, Sep 22, 2014 at 11:04 PM, Jeff King <peff@peff.net> wrote:
Show 9 quoted lines
>
> I don't mind silencing this one warning (even though I find it a little
> ridiculous). I'm slightly concerned that more brain-damage may be coming
> our way, but we can deal with that if it ever does.
>
> Like Junio, I prefer keeping strlen() rather than switching to sizeof,
> as it is less error-prone (no need for extra "-1" dance, and it won't
> silently do the wrong thing if the array is ever converted to a
> pointer).
I actually do not mind losing the sample[] array too much.

The early 45 bytes or so of that array (or a string constant) is not used by the code at all; I didn't want to count "From " (that's 5), 40-hex and then a SP -- ah, see, it is 46 bytes and I didn't want such miscounting. The only real contents that matter in that sample[] array is the tail part that is meant as the magic(5) cue. I'd be OK if the code checked the length of the line against a hardcoded constant and then did strcmp() starting from a hardcoded offset of the string and the magic cue string, and that would also avoid the warning from Eric's compiler.

But personally, I think the way it is coded is much easier to read, and is much harder to get it wrong while maintaining it. So...

Jeff King· Sep 23, 2014, 07:51 UTC · re: Junio C Hamano · lore

Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning

On Mon, Sep 22, 2014 at 11:26:14PM -0700, Junio C Hamano wrote:
Show 24 quoted lines
> On Mon, Sep 22, 2014 at 11:04 PM, Jeff King <peff@peff.net> wrote:
> >
> > I don't mind silencing this one warning (even though I find it a little
> > ridiculous). I'm slightly concerned that more brain-damage may be coming
> > our way, but we can deal with that if it ever does.
> >
> > Like Junio, I prefer keeping strlen() rather than switching to sizeof,
> > as it is less error-prone (no need for extra "-1" dance, and it won't
> > silently do the wrong thing if the array is ever converted to a
> > pointer).
> 
> I actually do not mind losing the sample[] array too much.
> 
> The early 45 bytes or so of that array (or a string constant) is not used
> by the code at all; I didn't want to count "From " (that's 5), 40-hex and
> then a SP -- ah, see, it is 46 bytes and I didn't want such miscounting.
> The only real contents that matter in that sample[] array is the tail part
> that is meant as the magic(5) cue. I'd be OK if the code checked the
> length of the line against a hardcoded constant and then did strcmp()
> starting from a hardcoded offset of the string and the magic cue string,
> and that would also avoid the warning from Eric's compiler.
> 
> But personally, I think the way it is coded is much easier to read,
> and is much harder to get it wrong while maintaining it.  So...

I agree. I was going to suggest switching to a static const array instead of a string literal, but retaining strlen()...but I see you already queued that in pu. So if what is there works for Eric (I do not have the compiler in question to test with), that seems reasonable.

-Peff
Eric Sunshine· Sep 23, 2014, 08:05 UTC · re: Jeff King · lore

Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning

On Tue, Sep 23, 2014 at 3:51 AM, Jeff King <peff@peff.net> wrote:
Show 22 quoted lines
> On Mon, Sep 22, 2014 at 11:26:14PM -0700, Junio C Hamano wrote:
>
>> On Mon, Sep 22, 2014 at 11:04 PM, Jeff King <peff@peff.net> wrote:
>> >
>> > I don't mind silencing this one warning (even though I find it a little
>> > ridiculous). I'm slightly concerned that more brain-damage may be coming
>> > our way, but we can deal with that if it ever does.
>> >
>> > Like Junio, I prefer keeping strlen() rather than switching to sizeof,
>> > as it is less error-prone (no need for extra "-1" dance, and it won't
>> > silently do the wrong thing if the array is ever converted to a
>> > pointer).
>>
>> I actually do not mind losing the sample[] array too much.
>>
>> But personally, I think the way it is coded is much easier to read,
>> and is much harder to get it wrong while maintaining it.  So...
>
> I agree. I was going to suggest switching to a static const array
> instead of a string literal, but retaining strlen()...but I see you
> already queued that in pu. So if what is there works for Eric (I do not
> have the compiler in question to test with), that seems reasonable.

What is queued in 'pu' is the same as my patch [1] minus the superfluous strlen() => sizeof() change, so it works fine for me. Thanks.

[1]: http://article.gmane.org/gmane.comp.version-control.git/257345
Eric Sunshine· Sep 23, 2014, 07:58 UTC · re: Junio C Hamano · lore

Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning

On Tue, Sep 23, 2014 at 2:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 21 quoted lines
> On Mon, Sep 22, 2014 at 11:04 PM, Jeff King <peff@peff.net> wrote:
>>
>> I don't mind silencing this one warning (even though I find it a little
>> ridiculous). I'm slightly concerned that more brain-damage may be coming
>> our way, but we can deal with that if it ever does.
>>
>> Like Junio, I prefer keeping strlen() rather than switching to sizeof,
>> as it is less error-prone (no need for extra "-1" dance, and it won't
>> silently do the wrong thing if the array is ever converted to a
>> pointer).
>
> I actually do not mind losing the sample[] array too much.
>
> The early 45 bytes or so of that array (or a string constant) is not used
> by the code at all; I didn't want to count "From " (that's 5), 40-hex and
> then a SP -- ah, see, it is 46 bytes and I didn't want such miscounting.
> The only real contents that matter in that sample[] array is the tail part
> that is meant as the magic(5) cue. I'd be OK if the code checked the
> length of the line against a hardcoded constant and then did strcmp()
> starting from a hardcoded offset of the string and the magic cue string,
> and that would also avoid the warning from Eric's compiler.

The -Wstring-plus-int option is "smart" enough to suppress the warning if the hardcoded offset falls within the bounds of the string literal, so this could work (but it feels a bit fragile compared to the current code).

> But personally, I think the way it is coded is much easier to read,
> and is much harder to get it wrong while maintaining it.  So...
Eric Sunshine· Sep 23, 2014, 07:52 UTC · re: Jeff King · lore

Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning

On Tue, Sep 23, 2014 at 2:04 AM, Jeff King <peff@peff.net> wrote:
Show 27 quoted lines
> On Mon, Sep 22, 2014 at 05:10:08PM -0400, Eric Sunshine wrote:
>
>> On Mon, Sep 22, 2014 at 1:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> > Eric Sunshine <sunshine@sunshineco.com> writes:
>> >
>> >> The just-released Apple Xcode 6.0.1 has -Wstring-plus-int enabled by
>> >> default which complains about pointer arithmetic applied to a string
>> >> literal:
>> >>
>> >>     builtin/mailinfo.c:303:24: warning:
>> >>         adding 'long' to a string does not append to the string
>> >>             return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) ...
>> >>                            ~~~~~~~^~~~~~~~~~~~~
>> >
>> > And why is that a warning-worthy violation?
>>
>> Not being privy to Apple's decision making process, I can only guess
>> that it is in response to their new Swift programming language which
>> they are pushing heavily on iOS (and soon Mac OS X), in which '+' is
>> the string concatenation operator. For projects written in Swift and
>> incorporating legacy or portable components in C, C++, or Objective-C,
>> the warning may help programmer's avoid the pitfall of thinking that
>> '+' is also concatenation in the C-based languages.
>
> That is my reading from the warning text, too, but I have to wonder:
> wouldn't that mean they should be warning about pointer + pointer, not
> pointer + int?

'pointer + pointer' is not legal C, is it? What would the result represent? The compiler correctly diagnoses that case as an error (without special command-line switches):

    error: invalid operands to binary expression
      ('char *' and 'char *')
      return "a" + "b";
             ~~~ ^ ~~~
> Also, wouldn't the same advice apply to adding to _any_ char pointer,
> not just a string literal?

Not really. Indexing into a char array via pointer arithmetic is a perfectly reasonable and common idiom in C (indeed, git is peppered with it), so such a warning would be pure noise. The more restricted case of 'stringliteral + something' is probably sufficiently rare that diagnosing it may catch genuine errors by programmers coming from languages in which '+' does string concatenation (which may be the original motivation for the warning; see below).

> I know you don't have answers to those questions, but the whole thing
> seems rather silly to me.

I did some more research and discovered that -Wstring-plus-int was added in response to 'string + int' cases being found in Chromium's C code (perhaps by programmers living in both JavaScript and C worlds). The proposed option was discussed [1] and committed [2]. Later a -Wstring-plus-char option was also discussed [3] and committed [4].

Regarding Apple: The Swift language supports 'string + string' and 'string + char', but not 'string + int'. However, in C, the char would be promoted to int, so there is some sense in warning about 'string + int' if they want to help safeguard programmers coming to C from Swift (but, again, I'm just speculating).

[1]: http://thread.gmane.org/gmane.comp.compilers.clang.scm/47203/ [2]: http://llvm.org/klaus/clang/commit/1cb2d742eb6635aeab6132ee5f0b5781d39487d7/ [3]: http://thread.gmane.org/gmane.comp.compilers.clang.scm/82462/ [4]: http://llvm.org/klaus/clang/commit/02debf605cd904edac8dceb196e5f142ce3d14eb/

Show 9 quoted lines
>> > Can we have them fix their compiler instead?
>>
>> If the above supposition is correct, then it's likely that Apple
>> considers this a feature, not a bug which needs to be fixed.
>
> Like Junio, I prefer keeping strlen() rather than switching to sizeof,
> as it is less error-prone (no need for extra "-1" dance, and it won't
> silently do the wrong thing if the array is ever converted to a
> pointer).
Sounds reasonable.
Jeff King· Sep 23, 2014, 08:12 UTC · re: Eric Sunshine · lore

Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning

On Tue, Sep 23, 2014 at 03:52:21AM -0400, Eric Sunshine wrote:
Show 12 quoted lines
> > That is my reading from the warning text, too, but I have to wonder:
> > wouldn't that mean they should be warning about pointer + pointer, not
> > pointer + int?
> 
> 'pointer + pointer' is not legal C, is it? What would the result
> represent? The compiler correctly diagnoses that case as an error
> (without special command-line switches):
> 
>     error: invalid operands to binary expression
>       ('char *' and 'char *')
>       return "a" + "b";
>              ~~~ ^ ~~~

You're correct that it's not legal. My point was more that "pointer + int" is already clearly not string concatenation, because the operands are not two strings.

I think the answer here (from the threads you linked below) is that people expect it to not just concatenate, but also auto-convert integers to strings. Yeesh.

Show 6 quoted lines
> > Also, wouldn't the same advice apply to adding to _any_ char pointer,
> > not just a string literal?
> 
> Not really. Indexing into a char array via pointer arithmetic is a
> perfectly reasonable and common idiom in C (indeed, git is peppered
> with it), so such a warning would be pure noise.

That is a good reason not to do the warning in these cases (and why I hope that we will not have to deal with this further). But IMHO it is good evidence that the warning is not well thought-out. It seems silly that:

  x = "foo" + 1;
is bad, but:
  y = "foo";
  x = y + 1;

is not[1]. Saying the first one is rare does not seem like a good excuse; rather the existence of the second should tip you off that the idiom is valid and not to be complained about.

Anyway, there is not much point in me complaining further about it here. You are not the one who introduced it. :)

Thanks for digging in the history. It was interesting at least.
-Peff
[1] Actually, from reading the patch thread, I think the "1" needs to be
    a non-constant integer here. But that just furthers the point: if
    you have to neuter the warning to prevent a ton of false positives,
    that is a good indication that you should not be warning. :)
Junio C Hamano· Sep 22, 2014, 20:50 UTC · re: Eric Sunshine · lore

Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning

Eric Sunshine <sunshine@sunshineco.com> writes:
> however, the solution in this patch allows us drop a couple strlen()s in
> favor of sizeof()s.

It is actually not a very good justification when you know you care about the length of the string. A decent compiler ought to know the length of a constant string can be computed at the compilation time.

Let's at least not do that part of the change.
Eric Sunshine· Sep 22, 2014, 21:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] mailinfo: resolve -Wstring-plus-int warning

On Mon, Sep 22, 2014 at 4:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> however, the solution in this patch allows us drop a couple strlen()s in
>> favor of sizeof()s.
>
> It is actually not a very good justification when you know you care
> about the length of the string.  A decent compiler ought to know the
> length of a constant string can be computed at the compilation time.

Indeed, which is why I mentioned this only in the commentary rather than the commit message; but I was also thinking of some of the less-than-decent compilers with which git is sometimes built.

> Let's at least not do that part of the change.
I don't have strong feelings about it.

← back to recent threads