On Fri, 17 May 2024 13:57:58 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Thu, May 16, 2024 at 11:30:58AM +0200, Stefano Brivio wrote:Yes: if we assign a value that's not in the enum, I expect static checkers to complain. But also for humans: even with that ifdef, a reader would know right away what values that might have.On Tue, 14 May 2024 11:03:19 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote: [...]I don't object to that, but I have two questions - What's the advantage to using the explicit enum? Is that for the benefit of static checkers and/or compiler diagnostics?/** * struct flow_common - Common fields for packet flows + * @state: State of the flow table entry * @type: Type of packet flow */ struct flow_common { + uint8_t state;In this case, I would typically do (https://seitan.rocks/seitan/tree/common/gluten.h?id=5a9302bab9c9bb3d1577f04…): #ifdef __GNUC__ enum flow_state state:8; #else uint8_t state; #endifAFAIK C itself doesn't really treat enums any differently to integer types.Right.- What's the need for GNUC? Are enum bitfields a gnu extension?They're rather permitted by gcc's interpretation of the standard: https://gcc.gnu.org/onlinedocs/gcc-14.1.0/gcc/Structures-unions-enumeration… Allowable bit-field types other than _Bool, signed int, and unsigned int (C99 and C11 6.7.2.1). Other integer types, such as long int, and enumerated types are permitted even in strictly conforming mode. but C11 says (6.7.2.1): A bit-field shall have a type that is a qualified or unqualified version of _Bool, signed int, unsigned int, or some other implementation-defined type. It is implementation-defined whether atomic types are permitted. Is an enum a "version" of those? Maybe not. That was at least the interpretation adopted by older gcc versions, up to 4.7.4: https://gcc.gnu.org/onlinedocs/gcc-4.7.4/gcc/Structures-unions-enumerations… Allowable bit-field types other than _Bool, signed int, and unsigned int (C99 6.7.2.1). No other types are permitted in strictly conforming mode. Did that change from C99? Not really: A bit-field shall have a type that is a qualified or unqualified version of _Bool, signed int, unsigned int, or some other implementation-defined type.Even versus C11, which we already require?Yes, I would say it doesn't change things. Only C23 would improve that: https://open-std.org/JTC1/SC22/WG14/www/docs/n3030.htm#design-constant.type by allowing us to define the underlying type.It's unrelated, but:...and in any case we need to make sure to assign single values in the enum above: there are no guarantees that FLOW_STATE_ACTIVE is 3 otherwise (except for that static_assert(), but that's not its purpose).I'm not clear how this comment relates to the one before.AFAIK nothing in here (or the rest of the series) relies on the specific numeric values of the flow state values (although we do rely on them being ordered as written in some places).while we rely on the fact that no value is bigger than 255, I realised that the standards already guarantee that values start from 0 and every subsequent constant is defined as one more than the previous one, all the way from C90 to C11, so this would actually be fine. Sorry, I don't know exactly why I thought that wouldn't be the case, I was pretty sure of the opposite until I checked. -- Stefano
On Fri, May 17, 2024 at 01:00:44PM +0200, Stefano Brivio wrote:On Fri, 17 May 2024 13:57:58 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Ah, ok. Makes sense, I've made that change.On Thu, May 16, 2024 at 11:30:58AM +0200, Stefano Brivio wrote:Yes: if we assign a value that's not in the enum, I expect static checkers to complain. But also for humans: even with that ifdef, a reader would know right away what values that might have.On Tue, 14 May 2024 11:03:19 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote: [...]I don't object to that, but I have two questions - What's the advantage to using the explicit enum? Is that for the benefit of static checkers and/or compiler diagnostics?/** * struct flow_common - Common fields for packet flows + * @state: State of the flow table entry * @type: Type of packet flow */ struct flow_common { + uint8_t state;In this case, I would typically do (https://seitan.rocks/seitan/tree/common/gluten.h?id=5a9302bab9c9bb3d1577f04…): #ifdef __GNUC__ enum flow_state state:8; #else uint8_t state; #endifAFAIK C itself doesn't really treat enums any differently to integer types.Right.- What's the need for GNUC? Are enum bitfields a gnu extension?They're rather permitted by gcc's interpretation of the standard: https://gcc.gnu.org/onlinedocs/gcc-14.1.0/gcc/Structures-unions-enumeration… Allowable bit-field types other than _Bool, signed int, and unsigned int (C99 and C11 6.7.2.1). Other integer types, such as long int, and enumerated types are permitted even in strictly conforming mode. but C11 says (6.7.2.1): A bit-field shall have a type that is a qualified or unqualified version of _Bool, signed int, unsigned int, or some other implementation-defined type. It is implementation-defined whether atomic types are permitted. Is an enum a "version" of those? Maybe not. That was at least the interpretation adopted by older gcc versions, up to 4.7.4: https://gcc.gnu.org/onlinedocs/gcc-4.7.4/gcc/Structures-unions-enumerations… Allowable bit-field types other than _Bool, signed int, and unsigned int (C99 6.7.2.1). No other types are permitted in strictly conforming mode. Did that change from C99? Not really: A bit-field shall have a type that is a qualified or unqualified version of _Bool, signed int, unsigned int, or some other implementation-defined type.Even versus C11, which we already require?Yes, I would say it doesn't change things. Only C23 would improve that: https://open-std.org/JTC1/SC22/WG14/www/docs/n3030.htm#design-constant.type by allowing us to define the underlying type.Right, I was expecting that behaviour - the way we define FLOW_NUM_STATES and similar things in a bunch of places relies on this.It's unrelated, but:...and in any case we need to make sure to assign single values in the enum above: there are no guarantees that FLOW_STATE_ACTIVE is 3 otherwise (except for that static_assert(), but that's not its purpose).I'm not clear how this comment relates to the one before.AFAIK nothing in here (or the rest of the series) relies on the specific numeric values of the flow state values (although we do rely on them being ordered as written in some places).while we rely on the fact that no value is bigger than 255, I realised that the standards already guarantee that values start from 0 and every subsequent constant is defined as one more than the previous one, all the way from C90 to C11, so this would actually be fine.Sorry, I don't know exactly why I thought that wouldn't be the case, I was pretty sure of the opposite until I checked.Ok. I've scattered in some extra static_assert()s in the vicinity, too. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson