Use of enums, why?

[greg@kroah.com (Greg KH)]

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