Re: [PATCH] media: gspca/gl860-mi1320: avoid -Wstring-concatenation warning

[Arnd Bergmann]

On Mon, Oct 4, 2021 at 12:55 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>
> Hi Arnd,
>
> On 27/09/2021 14:20, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@xxxxxxxx>
> >
> > Newer clang versions are suspicious of definitions that mix concatenated
> > strings with comma-separated arrays of strings, this has found real bugs
> > elsewhere, but this seems to be a false positive:
> >
> > drivers/media/usb/gspca/gl860/gl860-mi1320.c:62:37: error: suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma? [-Werror,-Wstring-concatenation]
> >         "\xd3\x02\xd4\x28\xd5\x01\xd0\x02" "\xd1\x18\xd2\xc1"
> >                                            ^
> >                                           ,
> > drivers/media/usb/gspca/gl860/gl860-mi1320.c:62:2: note: place parentheses around the string literal to silence warning
> >         "\xd3\x02\xd4\x28\xd5\x01\xd0\x02" "\xd1\x18\xd2\xc1"
> >
> > Use the extra parentheses as suggested in the warning message.
>
> I noticed that gl860-ov9655.c uses the same construct, doesn't that produce the
> same warning?

Curiously, it does not. I tried reproducing this in godbolt.org,
see https://godbolt.org/z/W6K69qcz3

For some reason, clang only warns about some of those, and it appears to
depend on the ratio between string concatenations and array elements.

> Also, does clang only warn about 'static u8 *tbl[]' initializers, or also
> for 'static u8 *tbl' initializers (i.e. not a pointer array) with the same
> string concatenation?

When there is only one element rather than an array, it does not warn,
because it's not a mix of concatenation and array elements.

> I made a patch that replaces these ugly hex strings with compound initializers:
>
> static u8 *tbl_640[] = {
>         (u8[]){
>                 0x0d, 0x80, 0xf1, 0x08, 0x03, 0x04, 0xf1, 0x04,
>                 0x04, 0x05, 0xf1, 0x02, 0x07, 0x01, 0xf1, 0x7c,
>                 0x08, 0x00, 0xf1, 0x0e, 0x21, 0x80, 0xf1, 0x00,
>                 0x0d, 0x00, 0xf1, 0x08, 0xf0, 0x00, 0xf1, 0x01,
>                 0x34, 0x10, 0xf1, 0x10, 0x3a, 0x43, 0xf1, 0x00,
>                 0xa6, 0x05, 0xf1, 0x02, 0xa9, 0x04, 0xf1, 0x04,
>                 0xa7, 0x02, 0xf1, 0x81, 0xaa, 0x01, 0xf1, 0xe2,
>                 0xae, 0x0c, 0xf1, 0x09
>         }, (u8[]){
>                 0xf0, 0x00, 0xf1, 0x02, 0x39, 0x03, 0xf1, 0xfc,
>                 0x3b, 0x04, 0xf1, 0x04, 0x57, 0x01, 0xf1, 0xb6,
>                 0x58, 0x02, 0xf1, 0x0d, 0x5c, 0x1f, 0xf1, 0x19,
>                 0x5d, 0x24, 0xf1, 0x1e, 0x64, 0x5e, 0xf1, 0x1c,
>                 0xd2, 0x00, 0xf1, 0x00, 0xcb, 0x00, 0xf1, 0x01
>         }, (u8[]){
>                 0xd3, 0x02, 0xd4, 0x10, 0xd5, 0x81, 0xd0, 0x02,
>                 0xd1, 0x08, 0xd2, 0xe1
>         }
> };
>
> but it clang also warns about 'static u8 *tbl' multi-string initializers,
> then it would make sense to replace all these hex-strings. It's rather
> ugly.

This seems fine.

       Arnd