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... 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