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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Sep 23, 2014 at 2:04 AM, Jeff King <peff@xxxxxxxx> 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@xxxxxxxxx> wrote:
>> > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]