Re: [PATCH v4 4/8] bits: introduce fixed-type BIT

[David Laight]

On Wed, 5 Mar 2025 21:56:22 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

> On Thu, Mar 06, 2025 at 02:17:18AM +0900, Vincent Mailhol wrote:
> > On 06/03/2025 at 00:48, Andy Shevchenko wrote:  
> > > On Wed, Mar 05, 2025 at 11:48:10PM +0900, Vincent Mailhol wrote:  
> > >> On 05/03/2025 at 23:33, Andy Shevchenko wrote:  
> > >>> On Wed, Mar 05, 2025 at 10:00:16PM +0900, Vincent Mailhol via B4 Relay wrote:  
> 
> ...
> 
> > >>>> +#define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (unsigned int)BIT(b))
> > >>>> +#define BIT_U16(b) (BIT_INPUT_CHECK(u16, b) + (unsigned int)BIT(b))  
> > >>>
> > >>> Why not u8 and u16? This inconsistency needs to be well justified.  
> > >>
> > >> Because of the C integer promotion rules, if casted to u8 or u16, the
> > >> expression will immediately become a signed integer as soon as it is get
> > >> used. For example, if casted to u8
> > >>
> > >>   BIT_U8(0) + BIT_U8(1)
> > >>
> > >> would be a signed integer. And that may surprise people.  
> > > 
> > > Yes, but wouldn't be better to put it more explicitly like
> > > 
> > > #define BIT_U8(b)	(BIT_INPUT_CHECK(u8, b) + (u8)BIT(b) + 0 + UL(0)) // + ULL(0) ?  
> > 
> > OK, the final result would be unsigned. But, I do not follow how this is
> > more explicit.
> > 
> > Also, why doing:
> > 
> >   (u8)BIT(b) + 0 + UL(0)
> > 
> > and not just:
> > 
> >   (u8)BIT(b) + UL(0)
> > 
> > ?
> > 
> > What is that intermediary '+ 0' for?
> > 
> > I am sorry, but I am having a hard time understanding how casting to u8
> > and then doing an addition with an unsigned long is more explicit than
> > directly doing a cast to the desired type.  
> 
> Reading this again, I think we don't need it at all. u8, aka unsigned char,
> will be promoted to int, but it will be int with a value < 256, can't be signed
> as far as I understand this correctly.

The value can't be negative, but the type will be a signed one.
Anything comparing types (and there are a few) will treat it as signed.
It really is bad practise to even pretend you can have an expression
(rather that a variable) that has a type smaller than 'int'.
It wouldn't surprise me if even an 'a = b' assignment promotes 'b' to int.

So it is even questionable whether BIT8() and BIT16() should even exist at all.
There can be reasons to return 'unsigned int' rather than 'unsigned long'.
But with the type definitions that Linux uses (and can't really be changed)
you can have BIT32() that is 'unsigned int' and BIT64() that is 'unsigned long
long'. These are then the same on 32bit and 64bit.

	David