* Use of enums, why?
@ 2018-07-11 9:30 John Whitmore
2018-07-11 9:40 ` Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: John Whitmore @ 2018-07-11 9:30 UTC (permalink / raw
To: kernelnewbies
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;
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?
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 */
...
}
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.
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. 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.
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.
thanks for any thoughts or clarification
John
^ permalink raw reply [flat|nested] 3+ messages in thread
* Use of enums, why?
2018-07-11 9:30 Use of enums, why? John Whitmore
@ 2018-07-11 9:40 ` Greg KH
2018-07-11 14:00 ` John Whitmore
0 siblings, 1 reply; 3+ messages in thread
From: Greg KH @ 2018-07-11 9:40 UTC (permalink / raw
To: kernelnewbies
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Use of enums, why?
2018-07-11 9:40 ` Greg KH
@ 2018-07-11 14:00 ` John Whitmore
0 siblings, 0 replies; 3+ messages in thread
From: John Whitmore @ 2018-07-11 14:00 UTC (permalink / raw
To: kernelnewbies
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-07-11 14:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-07-11 9:30 Use of enums, why? John Whitmore
2018-07-11 9:40 ` Greg KH
2018-07-11 14:00 ` John Whitmore
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.