On Wed, Jul 11, 2018 at 11:40:18AM +0200, Greg KH wrote: > 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 Yes that clears that up. Thanks a million John _______________________________________________ Kernelnewbies mailing list Kernelnewbies@xxxxxxxxxxxxxxxxx https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
