Re: [PATCH -mm] -funsigned-char, x86: make struct p4_event_bind::cntr signed array

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Alexey Dobriyan <adobriyan@gmail.com>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>,
	akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
	mm-commits@vger.kernel.org, masahiroy@kernel.org,
	keescook@chromium.org, gregkh@linuxfoundation.org,
	andriy.shevchenko@linux.intel.com
Subject: Re: [PATCH -mm] -funsigned-char, x86: make struct p4_event_bind::cntr signed array
Date: Fri, 21 Oct 2022 10:23:59 -0700	[thread overview]
Message-ID: <CAHk-=wigjpfSxu4-JXrKK9gW7m35h4pgmOd1gY2Moe9VCrXh8Q@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=wgb3RYTPd24rXs8dRdiqiPAzq7uc4Suxu1On6_DTDnf5g@mail.gmail.com>

On Fri, Oct 21, 2022 at 10:11 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I think you can fix that by simply warning about character constants
> with the high bit set.
>
> Something like this..

This actually triggers for the kernel, and I think those (few)
warnings are likely worth just fixing.

Because things like

                put_tty_queue('\377', ldata);

just isn't worth it.

Or look at this horror:

                        if (c == (unsigned char) '\377' && I_PARMRK(tty))

where somebody did realize that '\377' might be -1, so they added the
"helpful" cast.

Wouldn't that be much nicer and simpler as just

                        if (c == 255 && I_PARMRK(tty))

instead? Or just 0377 if you really love octal in the context of
characters (and really, nobody should). Or 0xff.

At no point does "(unsigned char) '\377' " strike me as a really
readable way to write things. There's two of those things.

We also have

        memset(stack, '\xff', 64);

which really isn't helpful either. Why not just use 0xff, or even just -1.

We also have a lot of those in lib/hexdump.c, for no good reason,
particularly as those arrays are 'unsigned char[]' to begin with, not
'char'.

I really don't understand why people would use '\xAA' instead of just
using 0xAA.

We also have

        static char sample_rate_buffer[4] = { '\x80', '\xbb', '\x00', '\x00' };

in sound/usb/mixer_scarlett.c, and that should be a byte array, so the
'char' should probably be 'unsigned char' or 'u8' in the first place,
and again it would be just simpler and clearer to use plain hex
constants.

So that sparse warning looks simple enough, and I think every single
case was just the kernel being odd.

Now, in *string* constants, you sometimes do want to use that format, ie

    u8 array[] = "abcd\377";

is a reasonable way to avoid having to use a more complex initializer.
But that sparse patch of mine only complains about (non-wide)
character constants unless I screwed something up.

                  Linus