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

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

[Eric Sunshine]

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(-)

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

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

[Junio C Hamano]

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?  Can we have them fix
their compiler instead?

>
> 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,

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

[Eric Sunshine]

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.

> 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>

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

[Jeff King]

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?

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

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

[Junio C Hamano]

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...

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

[Jeff King]

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.
> 
> 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

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

[Eric Sunshine]

On Tue, Sep 23, 2014 at 3:51 AM, Jeff King <peff@peff.net> wrote:
> 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

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

[Eric Sunshine]

On Tue, Sep 23, 2014 at 2:26 AM, Junio C Hamano <gitster@pobox.com> 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.
>
> 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...

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

[Eric Sunshine]

On Tue, Sep 23, 2014 at 2:04 AM, Jeff King <peff@peff.net> wrote:
> 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/

>> > 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.

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

[Jeff King]

On Tue, Sep 23, 2014 at 03:52:21AM -0400, Eric Sunshine wrote:

> > 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.

> > 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. :)

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

[Junio C Hamano]

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.

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

[Eric Sunshine]

On Mon, Sep 22, 2014 at 4:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.