On Wed, Jul 11, 2018 at 10:30:28AM +0100, John Whitmore wrote: > I only learning the ropes, and might have missed the memo on the use of enums > so forgive me. I have looked at > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#macros-enums-and-rtl > but that didn't answer my question. > > I'm doing a clean up of staging:rtl8192u: clearing out checkpatch errors and > triming out data structures which aren't used in the code. So I came across a > typedef of an enumerated type in the file: > > drivers/staging/rtl8192u/r8192U.h > > typedef enum rf_optype { > RF_OP_By_SW_3wire = 0, > RF_OP_By_FW, > RF_OP_MAX > } rf_op_type; To fix this up for checkpatch, it should just look like: enum rf_optype { RF_OP_By_SW_3wire = 0, RF_OP_By_FW, RF_OP_MAX }; > A quick grep for this type in drivers/staging/rtl8192u directory shows that > the type is never used outside that header definition. So I can remove it? As you found out, no :) > Well no you can't because the values defined in the enum are used. > > In drivers/staging/rtl8192u/r8192U_core.c for example: > priv->Rf_Mode = RF_OP_By_SW_3wire; > > that element priv->Rf_Mode is defined in the structure > > typedef struct r8192_priv { > ... > u8 Rf_Mode; /* For Firmware RF -R/W switch */ Ah, that should be: enum rf_optype RF_Mode; right? > ... > } > > > So now to the question, as I understand it the compiler will use an int type > for the enumerated type. The data structure r8192_priv doesn't use this int type > because the programmer knows that a u8 will do to hold the 3 possible values > defined by the enumerated type. Or someone messed up and didn't realize that is what was holding those values :) > So you're saving a few bytes in a data structure, which I'm happy about, but > the point of enumerated types is, as I understand it, so that the compiler can > do some checking to ensure a value is not assigned in error. You are correct. > Since the enum isn't being used, there is no compiler type checking, > so why use an enumerated type? Might as well have used three #define > values. That too would work, but you loose the typechecking. > The obvious thing to do is leave well enough alone. But I had to ask what is > the correct implementation? Use the enum in the data structure, instead of > u8? Or just #define a few constants? > > #define RF_OP_By_SW_3wire 1 > #define RF_OP_By_FW 2 > #define RF_OP_MAX 3 > > Given the space saving of u8 over int I'd probably go with the #define. Guess > it depends on how many of those data structures are being declared. I'd stick to the define, UNLESS this structure is coming from/to the hardware directly. Then you need to use u8. So look and see if that is what is happening here or not. If it's just a "normal" structure in memory, then use an enum to keep the type safety. hope this helps, greg k-h _______________________________________________ Kernelnewbies mailing list Kernelnewbies@xxxxxxxxxxxxxxxxx https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
