Re: [Outreachy kernel] [PATCH v2 1/5] staging: r8723au: break parameter list at separators and align to open braces

[Alison Schofield <amsfield22@gmail.com>]

On Sat, Oct 24, 2015 at 07:40:29PM -0700, Greg KH wrote:
> On Tue, Oct 20, 2015 at 11:01:58PM -0700, Alison Schofield wrote:
> > Improve readability by aligning the struct cmd_hdl wlancmds[] to fit
> > within 80 chars and cleaning up the adjacent index comments.
> > 
> > Addresses checkpatch.pl WARNING: line over 80 characters
> > 
> > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > ---
> >  drivers/staging/rtl8723au/core/rtw_cmd.c | 64 +++++++++++++++++++-------------
> >  1 file changed, 38 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8723au/core/rtw_cmd.c b/drivers/staging/rtl8723au/core/rtw_cmd.c
> > index 46aea16..c3579ac 100644
> > --- a/drivers/staging/rtl8723au/core/rtw_cmd.c
> > +++ b/drivers/staging/rtl8723au/core/rtw_cmd.c
> > @@ -22,7 +22,7 @@
> >  #include <rtw_sreset.h>
> >  
> >  static struct cmd_hdl wlancmds[] = {
> > -	GEN_DRV_CMD_HANDLER(0, NULL) /*0*/
> > +	GEN_DRV_CMD_HANDLER(0, NULL)					/*0*/
> >  	GEN_DRV_CMD_HANDLER(0, NULL)
> >  	GEN_DRV_CMD_HANDLER(0, NULL)
> >  	GEN_DRV_CMD_HANDLER(0, NULL)
> > @@ -32,18 +32,26 @@ static struct cmd_hdl wlancmds[] = {
> >  	GEN_MLME_EXT_HANDLER(0, NULL)
> >  	GEN_MLME_EXT_HANDLER(0, NULL)
> >  	GEN_MLME_EXT_HANDLER(0, NULL)
> > -	GEN_MLME_EXT_HANDLER(0, NULL) /*10*/
> > +	GEN_MLME_EXT_HANDLER(0, NULL)					/*10*/
> >  	GEN_MLME_EXT_HANDLER(0, NULL)
> >  	GEN_MLME_EXT_HANDLER(0, NULL)
> >  	GEN_MLME_EXT_HANDLER(0, NULL)
> > -	GEN_MLME_EXT_HANDLER(sizeof(struct wlan_bssid_ex), join_cmd_hdl23a) /*14*/
> > -	GEN_MLME_EXT_HANDLER(sizeof(struct disconnect_parm), disconnect_hdl23a)
> > -	GEN_MLME_EXT_HANDLER(sizeof(struct wlan_bssid_ex), createbss_hdl23a)
> > -	GEN_MLME_EXT_HANDLER(sizeof(struct setopmode_parm), setopmode_hdl23a)
> > -	GEN_MLME_EXT_HANDLER(sizeof(struct sitesurvey_parm), sitesurvey_cmd_hdl23a) /*18*/
> > -	GEN_MLME_EXT_HANDLER(sizeof(struct setauth_parm), setauth_hdl23a)
> > -	GEN_MLME_EXT_HANDLER(sizeof(struct setkey_parm), setkey_hdl23a) /*20*/
> > -	GEN_MLME_EXT_HANDLER(sizeof(struct set_stakey_parm), set_stakey_hdl23a)
> > +	GEN_MLME_EXT_HANDLER(sizeof(struct wlan_bssid_ex),
> > +			     join_cmd_hdl23a)
> > +	GEN_MLME_EXT_HANDLER(sizeof(struct disconnect_parm),
> > +			     disconnect_hdl23a)
> > +	GEN_MLME_EXT_HANDLER(sizeof(struct wlan_bssid_ex),
> > +			     createbss_hdl23a)
> > +	GEN_MLME_EXT_HANDLER(sizeof(struct setopmode_parm),
> > +			     setopmode_hdl23a)
> > +	GEN_MLME_EXT_HANDLER(sizeof(struct sitesurvey_parm),
> > +			     sitesurvey_cmd_hdl23a)
> > +	GEN_MLME_EXT_HANDLER(sizeof(struct setauth_parm),
> > +			     setauth_hdl23a)
> > +	GEN_MLME_EXT_HANDLER(sizeof(struct setkey_parm),		/*20*/
> > +			     setkey_hdl23a)
> > +	GEN_MLME_EXT_HANDLER(sizeof(struct set_stakey_parm),
> > +			     set_stakey_hdl23a)
> 
> 
> This seems harder to read to me than before, how about you?
>
It does look worse in the diff, but when I'm scanning the entire 62 item
list, keeping the items to the left and the index comments to the right
seemed to make it easier to scan and find the item I want. (A maintainer
probably wouldn't be reading this list top to bottom.)

Is it appropriate for me to ask reviewers (you) to apply the patch to
review it?  It seems there is not always enough context in the diff.

alison