lkml.org 
[lkml]   [2020]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] staging: rtl8192u: Corrects 'Avoid CamelCase' for variables
From
Date
On Tue, 2020-03-17 at 16:43 +0300, Dan Carpenter wrote:
> On Tue, Mar 17, 2020 at 08:51:30AM +0000, Camylla Goncalves Cantanheide wrote:
> > The variables of function setKey triggered a 'Avoid CamelCase'
> > warning from checkpatch.pl. This patch renames these
> > variables to correct this warning.
> >
> > Signed-off-by: Camylla Goncalves Cantanheide <c.cantanheide@gmail.com>
> > ---
> > drivers/staging/rtl8192u/r8192U_core.c | 52 +++++++++++++-------------
> > 1 file changed, 26 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
> > index 93a15d57e..fcfb9024a 100644
> > --- a/drivers/staging/rtl8192u/r8192U_core.c
> > +++ b/drivers/staging/rtl8192u/r8192U_core.c
> > @@ -4877,50 +4877,50 @@ void EnableHWSecurityConfig8192(struct net_device *dev)
> > write_nic_byte(dev, SECR, SECR_value);
> > }
> >
> > -void setKey(struct net_device *dev, u8 EntryNo, u8 KeyIndex, u16 KeyType,
> > - u8 *MacAddr, u8 DefaultKey, u32 *KeyContent)
> > +void setKey(struct net_device *dev, u8 entryno, u8 keyindex, u16 keytype,
> > + u8 *macaddr, u8 defaultkey, u32 *keycontent)
> > {
> > - u32 TargetCommand = 0;
> > - u32 TargetContent = 0;
> > - u16 usConfig = 0;
> > + u32 target_command = 0;
> > + u32 target_content = 0;
> > + u16 us_config = 0;
>
> Use these renames to think deeply about naming.
>
> I don't like "entryno". I would prefer "entry_no". Use the same
> underscore for spaces rule for key_index, mac_addr and all the rest. Is
> "key_idx" better or "key_index"?
>
> What added value or meaning does the "target_" part of "target_command"
> add? Use "cmd" instead of "command". "target_command" and
> "target_content" are the same length and mostly the same letters. Avoid
> that sort of thing because it makes it hard to read at a glance. The
> two get swapped in your head.
>
> What does the "us_" mean in us_config? Is it microsecond as in usec?
> Is it United states? Actually it turns out it probably means "unsigned
> short". Never make the variable names show the type. If you have a
> good editor you can just hover the mouse over a variable to see the
> type. Or if you're using vim like me, then you have to use '*' to
> highlight the variable and scroll to the top of the function. Either
> way, never use "us_" to mean unsigned short.
>
> What does the "config" part of "us_config" mean? What does the "content"
> part of "target_content" mean? Always think about that. Variable names
> are hard and maybe "config" and "content" are clear enough. But at
> think about it, and consider all the options.
>
> Anyway, the reason that this patch needs to be re-written is because
> we want underscores in place of spaces for "key_type" and because
> "us_config" is against the rules. The rest is just something to
> consider and if you find better names, then go with that but if you
> don't just fix those two things and resend.

What Dan said and:

Make sure to successfully compile the source files the patch
modifies before sending the patch.

Renaming function arguments and not renaming the uses of the
arguments in the function is not good.


\
 
 \ /
  Last update: 2020-03-17 16:06    [W:0.043 / U:0.980 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site