diff mbox

[OPW,kernel] staging:vt6655:fix checkpatch warnings

Message ID 20140915170029.GA10565@kato-K52F
State New, archived
Headers show

Commit Message

Rajbinder Brar Sept. 15, 2014, 5 p.m. UTC
This patch fixes following checkpatch.pl warnings
WARNING: line over 80 characters

Signed-off-by: Rajbinder Brar <brar.rajbinder@gmail.com>
---
 drivers/staging/vt6655/80211hdr.h | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Julia Lawall Sept. 15, 2014, 5:05 p.m. UTC | #1
Sarah suggested in response to anther patch that one should use a more
descriptive subject.  Checkpatch warns about many things.  Here it could
be Fix line over 80 characters warnings.  Also pay attention to the
information about the context of the file affected.  Normally the : is
followed by a space.  You can see what others have done for the current
file using

git log --oneline -- file

julia

On Mon, 15 Sep 2014, Rajbinder Brar wrote:

> This patch fixes following checkpatch.pl warnings
> WARNING: line over 80 characters
>
> Signed-off-by: Rajbinder Brar <brar.rajbinder@gmail.com>
> ---
>  drivers/staging/vt6655/80211hdr.h | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/vt6655/80211hdr.h b/drivers/staging/vt6655/80211hdr.h
> index 4ddd22f..907b5c9 100644
> --- a/drivers/staging/vt6655/80211hdr.h
> +++ b/drivers/staging/vt6655/80211hdr.h
> @@ -156,8 +156,10 @@
>
>  /* GET & SET Frame Control bit */
>  #define WLAN_GET_FC_PRVER(n)    (((unsigned short)(n) >> 8) & (BIT0 | BIT1))
> -#define WLAN_GET_FC_FTYPE(n)    ((((unsigned short)(n) >> 8) & (BIT2 | BIT3)) >> 2)
> -#define WLAN_GET_FC_FSTYPE(n)   ((((unsigned short)(n) >> 8) & (BIT4|BIT5|BIT6|BIT7)) >> 4)
> +#define WLAN_GET_FC_FTYPE(n)    ((((unsigned short)(n) >> 8) & \
> +				(BIT2 | BIT3)) >> 2)
> +#define WLAN_GET_FC_FSTYPE(n)   ((((unsigned short)(n) >> 8) & \
> +				(BIT4|BIT5|BIT6|BIT7)) >> 4)
>  #define WLAN_GET_FC_TODS(n)     ((((unsigned short)(n) << 8) & (BIT8)) >> 8)
>  #define WLAN_GET_FC_FROMDS(n)   ((((unsigned short)(n) << 8) & (BIT9)) >> 9)
>  #define WLAN_GET_FC_MOREFRAG(n) ((((unsigned short)(n) << 8) & (BIT10)) >> 10)
> @@ -168,8 +170,10 @@
>  #define WLAN_GET_FC_ORDER(n)    ((((unsigned short)(n) << 8) & (BIT15)) >> 15)
>
>  /* Sequence Field bit */
> -#define WLAN_GET_SEQ_FRGNUM(n) (((unsigned short)(n) >> 8) & (BIT0|BIT1|BIT2|BIT3))
> -#define WLAN_GET_SEQ_SEQNUM(n) ((((unsigned short)(n) >> 8) & (~(BIT0|BIT1|BIT2|BIT3))) >> 4)
> +#define WLAN_GET_SEQ_FRGNUM(n) (((unsigned short)(n) >> 8) & \
> +				(BIT0|BIT1|BIT2|BIT3))
> +#define WLAN_GET_SEQ_SEQNUM(n) ((((unsigned short)(n) >> 8) & \
> +				(~(BIT0|BIT1|BIT2|BIT3))) >> 4)
>
>  /* Capability Field bit */
>  #define WLAN_GET_CAP_INFO_ESS(n)           (((n) >> 8) & BIT0)
> @@ -190,7 +194,8 @@
>  /* GET & SET Frame Control bit */
>  #define WLAN_GET_FC_PRVER(n)    (((unsigned short)(n)) & (BIT0 | BIT1))
>  #define WLAN_GET_FC_FTYPE(n)    ((((unsigned short)(n)) & (BIT2 | BIT3)) >> 2)
> -#define WLAN_GET_FC_FSTYPE(n)   ((((unsigned short)(n)) & (BIT4|BIT5|BIT6|BIT7)) >> 4)
> +#define WLAN_GET_FC_FSTYPE(n)   ((((unsigned short)(n)) & \
> +				(BIT4|BIT5|BIT6|BIT7)) >> 4)
>  #define WLAN_GET_FC_TODS(n)     ((((unsigned short)(n)) & (BIT8)) >> 8)
>  #define WLAN_GET_FC_FROMDS(n)   ((((unsigned short)(n)) & (BIT9)) >> 9)
>  #define WLAN_GET_FC_MOREFRAG(n) ((((unsigned short)(n)) & (BIT10)) >> 10)
> @@ -202,7 +207,8 @@
>
>  /* Sequence Field bit */
>  #define WLAN_GET_SEQ_FRGNUM(n) (((unsigned short)(n)) & (BIT0|BIT1|BIT2|BIT3))
> -#define WLAN_GET_SEQ_SEQNUM(n) ((((unsigned short)(n)) & (~(BIT0|BIT1|BIT2|BIT3))) >> 4)
> +#define WLAN_GET_SEQ_SEQNUM(n) ((((unsigned short)(n)) & \
> +				(~(BIT0|BIT1|BIT2|BIT3))) >> 4)
>
>  /* Capability Field bit */
>  #define WLAN_GET_CAP_INFO_ESS(n)           ((n) & BIT0)
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "opw-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to opw-kernel+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
Julia Lawall Sept. 15, 2014, 5:12 p.m. UTC | #2
>  #define WLAN_GET_FC_PRVER(n)    (((unsigned short)(n) >> 8) & (BIT0 | BIT1))
> -#define WLAN_GET_FC_FTYPE(n)    ((((unsigned short)(n) >> 8) & (BIT2 | BIT3)) >> 2)
> -#define WLAN_GET_FC_FSTYPE(n)   ((((unsigned short)(n) >> 8) & (BIT4|BIT5|BIT6|BIT7)) >> 4)
> +#define WLAN_GET_FC_FTYPE(n)    ((((unsigned short)(n) >> 8) & \
> +				(BIT2 | BIT3)) >> 2)
> +#define WLAN_GET_FC_FSTYPE(n)   ((((unsigned short)(n) >> 8) & \
> +				(BIT4|BIT5|BIT6|BIT7)) >> 4)

Also, here I am not sure if this is the best solution.  Now eg

(BIT2 | BIT3)) >> 2)

is lined up with

((((unsigned short)(n) >> 8) &

It could look like (BIT2 | BIT3)) >> 2) is the right argument of &.  Bt
this is not the case.  The central operator of the expression is >>, and
it's right argument is 2.  So the code could be easy to misinterpret.

Perhaps a solution would be to line up (BIT2 | BIT3)) >> 2) with the
left side of the left argument of &.  At least someone looking at the code
might realize that something particular was going on.

Perhaps someone else has a better suggestion.

julia
Bob Copeland Sept. 15, 2014, 5:25 p.m. UTC | #3
On Mon, Sep 15, 2014 at 07:12:25PM +0200, Julia Lawall wrote:
> >  #define WLAN_GET_FC_PRVER(n)    (((unsigned short)(n) >> 8) & (BIT0 | BIT1))
> > -#define WLAN_GET_FC_FTYPE(n)    ((((unsigned short)(n) >> 8) & (BIT2 | BIT3)) >> 2)
> > -#define WLAN_GET_FC_FSTYPE(n)   ((((unsigned short)(n) >> 8) & (BIT4|BIT5|BIT6|BIT7)) >> 4)
> > +#define WLAN_GET_FC_FTYPE(n)    ((((unsigned short)(n) >> 8) & \
> > +				(BIT2 | BIT3)) >> 2)
> > +#define WLAN_GET_FC_FSTYPE(n)   ((((unsigned short)(n) >> 8) & \
> > +				(BIT4|BIT5|BIT6|BIT7)) >> 4)
> 
> Also, here I am not sure if this is the best solution.  Now eg
> 
> (BIT2 | BIT3)) >> 2)
> 
> is lined up with
> 
> ((((unsigned short)(n) >> 8) &
> 
> It could look like (BIT2 | BIT3)) >> 2) is the right argument of &.  Bt
> this is not the case.  The central operator of the expression is >>, and
> it's right argument is 2.  So the code could be easy to misinterpret.
> 
> Perhaps a solution would be to line up (BIT2 | BIT3)) >> 2) with the
> left side of the left argument of &.  At least someone looking at the code
> might realize that something particular was going on.
> 
> Perhaps someone else has a better suggestion.

In this case I think sticking to the 80-col rule hurts readability
overall.

However, at least according to my quick grep, I don't see these definitions
used in the driver at all, so simply removing them (after compile tests and so
forth) is probably the best way.

For things that are in use, conversion to macros in include/linux/ieee80211.h
would be nice.
diff mbox

Patch

diff --git a/drivers/staging/vt6655/80211hdr.h b/drivers/staging/vt6655/80211hdr.h
index 4ddd22f..907b5c9 100644
--- a/drivers/staging/vt6655/80211hdr.h
+++ b/drivers/staging/vt6655/80211hdr.h
@@ -156,8 +156,10 @@ 
 
 /* GET & SET Frame Control bit */
 #define WLAN_GET_FC_PRVER(n)    (((unsigned short)(n) >> 8) & (BIT0 | BIT1))
-#define WLAN_GET_FC_FTYPE(n)    ((((unsigned short)(n) >> 8) & (BIT2 | BIT3)) >> 2)
-#define WLAN_GET_FC_FSTYPE(n)   ((((unsigned short)(n) >> 8) & (BIT4|BIT5|BIT6|BIT7)) >> 4)
+#define WLAN_GET_FC_FTYPE(n)    ((((unsigned short)(n) >> 8) & \
+				(BIT2 | BIT3)) >> 2)
+#define WLAN_GET_FC_FSTYPE(n)   ((((unsigned short)(n) >> 8) & \
+				(BIT4|BIT5|BIT6|BIT7)) >> 4)
 #define WLAN_GET_FC_TODS(n)     ((((unsigned short)(n) << 8) & (BIT8)) >> 8)
 #define WLAN_GET_FC_FROMDS(n)   ((((unsigned short)(n) << 8) & (BIT9)) >> 9)
 #define WLAN_GET_FC_MOREFRAG(n) ((((unsigned short)(n) << 8) & (BIT10)) >> 10)
@@ -168,8 +170,10 @@ 
 #define WLAN_GET_FC_ORDER(n)    ((((unsigned short)(n) << 8) & (BIT15)) >> 15)
 
 /* Sequence Field bit */
-#define WLAN_GET_SEQ_FRGNUM(n) (((unsigned short)(n) >> 8) & (BIT0|BIT1|BIT2|BIT3))
-#define WLAN_GET_SEQ_SEQNUM(n) ((((unsigned short)(n) >> 8) & (~(BIT0|BIT1|BIT2|BIT3))) >> 4)
+#define WLAN_GET_SEQ_FRGNUM(n) (((unsigned short)(n) >> 8) & \
+				(BIT0|BIT1|BIT2|BIT3))
+#define WLAN_GET_SEQ_SEQNUM(n) ((((unsigned short)(n) >> 8) & \
+				(~(BIT0|BIT1|BIT2|BIT3))) >> 4)
 
 /* Capability Field bit */
 #define WLAN_GET_CAP_INFO_ESS(n)           (((n) >> 8) & BIT0)
@@ -190,7 +194,8 @@ 
 /* GET & SET Frame Control bit */
 #define WLAN_GET_FC_PRVER(n)    (((unsigned short)(n)) & (BIT0 | BIT1))
 #define WLAN_GET_FC_FTYPE(n)    ((((unsigned short)(n)) & (BIT2 | BIT3)) >> 2)
-#define WLAN_GET_FC_FSTYPE(n)   ((((unsigned short)(n)) & (BIT4|BIT5|BIT6|BIT7)) >> 4)
+#define WLAN_GET_FC_FSTYPE(n)   ((((unsigned short)(n)) & \
+				(BIT4|BIT5|BIT6|BIT7)) >> 4)
 #define WLAN_GET_FC_TODS(n)     ((((unsigned short)(n)) & (BIT8)) >> 8)
 #define WLAN_GET_FC_FROMDS(n)   ((((unsigned short)(n)) & (BIT9)) >> 9)
 #define WLAN_GET_FC_MOREFRAG(n) ((((unsigned short)(n)) & (BIT10)) >> 10)
@@ -202,7 +207,8 @@ 
 
 /* Sequence Field bit */
 #define WLAN_GET_SEQ_FRGNUM(n) (((unsigned short)(n)) & (BIT0|BIT1|BIT2|BIT3))
-#define WLAN_GET_SEQ_SEQNUM(n) ((((unsigned short)(n)) & (~(BIT0|BIT1|BIT2|BIT3))) >> 4)
+#define WLAN_GET_SEQ_SEQNUM(n) ((((unsigned short)(n)) & \
+				(~(BIT0|BIT1|BIT2|BIT3))) >> 4)
 
 /* Capability Field bit */
 #define WLAN_GET_CAP_INFO_ESS(n)           ((n) & BIT0)