From: Alex Elder
> Sent: 03 June 2021 22:46
>
> On 6/3/21 4:22 PM, David Laight wrote:
...
> >>>> --- a/drivers/staging/greybus/gpio.c
> >>>> +++ b/drivers/staging/greybus/gpio.c
> >>>> @@ -20,9 +20,9 @@
> >>>> struct gb_gpio_line {
> >>>> /* The following has to be an array of line_max entries */
> >>>> /* --> make them just a flags field */
> >>>> - u8 active: 1,
> >>>> - direction: 1, /* 0 = output, 1 = input */
> >>>> - value: 1; /* 0 = low, 1 = high */
> >>>> + u8 active:1,
> >>>> + direction:1, /* 0 = output, 1 = input */
> >>>> + value:1; /* 0 = low, 1 = high */
> >
> > Why are you even using bitfields at all?
> > If you cared about the structure size you'd not have a byte-size pad here.
>
> Apparently I committed this, and it was part of the very first
> Greybus drivers...
>
> These would be better defined as Booleans; there are others in
> the same structure after all. That would have avoided the
> checkpatch problem in the first place.
Using 'u8' can be sensible.
Boolean will be 32bit.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 6/3/21 4:48 PM, David Laight wrote:
> From: Alex Elder
>> Sent: 03 June 2021 22:46
>>
>> On 6/3/21 4:22 PM, David Laight wrote:
> ...
>>>>>> --- a/drivers/staging/greybus/gpio.c
>>>>>> +++ b/drivers/staging/greybus/gpio.c
>>>>>> @@ -20,9 +20,9 @@
>>>>>> struct gb_gpio_line {
>>>>>> /* The following has to be an array of line_max entries */
>>>>>> /* --> make them just a flags field */
>>>>>> - u8 active: 1,
>>>>>> - direction: 1, /* 0 = output, 1 = input */
>>>>>> - value: 1; /* 0 = low, 1 = high */
>>>>>> + u8 active:1,
>>>>>> + direction:1, /* 0 = output, 1 = input */
>>>>>> + value:1; /* 0 = low, 1 = high */
>>>
>>> Why are you even using bitfields at all?
>>> If you cared about the structure size you'd not have a byte-size pad here.
>>
>> Apparently I committed this, and it was part of the very first
>> Greybus drivers...
>>
>> These would be better defined as Booleans; there are others in
>> the same structure after all. That would have avoided the
>> checkpatch problem in the first place.
>
> Using 'u8' can be sensible.
> Boolean will be 32bit.
Not necessarily, sizeof(bool) is implementation defined.
And I thought you didn't think the size of the structure
was very important...
In any case, I'm open to changing the type of these fields,
and my preference would be bool rather than u8, because it
is completely clear what it represents.
-Alex
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
From: Alex Elder
> Sent: 03 June 2021 22:55
...
> Not necessarily, sizeof(bool) is implementation defined.
> And I thought you didn't think the size of the structure
> was very important...
It is 'implementation defined' but will be 32 bits on everything
except an old 32bit ARM ABI.
> In any case, I'm open to changing the type of these fields,
> and my preference would be bool rather than u8, because it
> is completely clear what it represents.
Yes, and it isn't at all clear what it actually means.
If the value of a bool memory location isn't 0 or 1
what does 'bool_a & bool_b' mean.
It might be 'undefined behaviour' - but that doesn't actually
exclude an ICBM hitting the coder's house!
I've seen very silly code generated (by an old gcc) for
simple statements like:
bool_a |= bool_b;
Mostly because it didn't trust the values to be 0 or 1
and wanted to ensure the result was either 0 or 1.
If I use an integer type (as in traditional C) I know
what I'm getting and there are no unexpected comparisons
and conditionals.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Fri, 2021-06-04 at 08:13 +0000, David Laight wrote:
> From: Alex Elder
> > Sent: 03 June 2021 22:55
> ...
> > Not necessarily, sizeof(bool) is implementation defined.
> > And I thought you didn't think the size of the structure
> > was very important...
>
> It is 'implementation defined' but will be 32 bits on everything
> except an old 32bit ARM ABI.
Really? (x86-64)
$ gcc -x c -
#include <stdio.h>
#include <stdlib.h>
struct foo {
_Bool b;
};
int main(int argc, char **argv)
{
printf("sizeof _Bool: %zu\n", sizeof(_Bool));
printf("sizeof struct foo: %zu\n", sizeof(struct foo));
return 0;
}
$ ./a.out
sizeof _Bool: 1
sizeof struct foo: 1
From: Joe Perches
> Sent: 04 June 2021 09:27
>
> On Fri, 2021-06-04 at 08:13 +0000, David Laight wrote:
> > From: Alex Elder
> > > Sent: 03 June 2021 22:55
> > ...
> > > Not necessarily, sizeof(bool) is implementation defined.
> > > And I thought you didn't think the size of the structure
> > > was very important...
> >
> > It is 'implementation defined' but will be 32 bits on everything
> > except an old 32bit ARM ABI.
>
> Really? (x86-64)
>
...
> sizeof _Bool: 1
> sizeof struct foo: 1
Gah, not enough coffee.
I said I don't like _Bool :-)
The old arm32 must have had 32bit bool.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 6/4/21 3:13 AM, David Laight wrote:
> Yes, and it isn't at all clear what it actually means.
> If the value of a bool memory location isn't 0 or 1
> what does 'bool_a & bool_b' mean.
I think this discussion is done, but I wanted to point out
that the above expression is incorrect. It might be valid,
but it would be bad code. A Boolean, when properly used,
should only be compared with true and false (or the result
of another Boolean expression). Therefore "&" is not the
right operator, "&&" is. The reason is similar to why you
would never use ~bool_a, you use !bool_a to invert its value.
-Alex