[cfe-commits] [patch] Warn in string literal + integral

[Aaron Ballman]

On Tue, Feb 21, 2012 at 9:17 PM, Nico Weber <thakis at chromium.org> wrote:
> Hi,
>
> the attached patch adds -Wstring-plus-int (I'm open to more creative
> names :-) ), which warns on code like |"error code: " + err|. The
> warning found 4 bugs in 2 files in chromium. The bugs it found look
> like this:
>
> File 1:
>    if (!read_node.InitByIdLookup(it->id)) {
>      error_handler()->OnUnrecoverableError(FROM_HERE, "Failed to look up "
>          " data for received change with id " + it->id);
>      return;
>    }
>
> File 2:
>    ResetStream(stream_id, spdy::INVALID_ASSOCIATED_STREAM,
>      "Received OnSyn with inactive associated stream " +
>      associated_stream_id);
> (Fix: http://codereview.chromium.org/9372076/diff/1/net/spdy/spdy_session.cc )
>
> (A coworker found the bug in the one file, which prompted me writing
> the warning. The warning then found the bug in the other file.)
>
>
> When building all of chromium and its many dependencies, the warning
> did also find 3 false positives, but they all look alike: Ffmepg
> contains three different functions that all look like this:
>
> const char *avformat_license(void)
> {
> #define LICENSE_PREFIX "libavformat license: "
>    return LICENSE_PREFIX FFMPEG_LICENSE + sizeof(LICENSE_PREFIX) - 1;
> }
>
> I don't quite get what this is for (a coworker suggested it's so that
> "libavformat license: " shows up in `strings` output, but is omitted
> from the return), but I think warning on code like this is kind of ok.
> Here's how the warning looks in this case:
>
> ../../third_party/ffmpeg/patched-ffmpeg/libavformat/utils.c:68:42:
> warning: Adding an integral to a string does not append to the string
> [-Wstring-plus-int]
>    return LICENSE_PREFIX FFMPEG_LICENSE + sizeof(LICENSE_PREFIX) - 1;
>           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
> ../../third_party/ffmpeg/patched-ffmpeg/libavformat/utils.c:68:42:
> note: use &(str[index]) if pointer arithmetic is intended
> 1 warning generated.
>
>
> I'm looking forward to comments :-)

Patch LGTM

~Aaron