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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 23, 2014, 07:52 UTC
Message-ID: <CAPig+cQmbOs7Xw8wv63mLHfpG13Vo+tR7oLq-5srCcP1QQddnQ@mail.gmail.com>
In-Reply-To: <20140923060407.GA23861@peff.net>

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.