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

From: Jeff King <peff@peff.net>
Date: Sep 23, 2014, 08:12 UTC
Message-ID: <20140923081225.GB11104@peff.net>
In-Reply-To: <CAPig+cQmbOs7Xw8wv63mLHfpG13Vo+tR7oLq-5srCcP1QQddnQ@mail.gmail.com>

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