Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: Move closing braces of 'if' statements to end of line

[Dafna Hirschfeld <dafna3@gmail.com>]

On Sat, Feb 17, 2018 at 06:27:34PM +0100, Julia Lawall wrote:
> 
> 
> On Sat, 17 Feb 2018, Dafna Hirschfeld wrote:
> 
> > Move all closing braces to the end of the line.
> > Remove braces in single 'if' statement.
> 
> It's not really a single if statement, but rather an if with a single
> statement in the then branch (or in both branches, as appropriate).
> 
> > Break 'if' condition to avoid 80 characters line.
> > Issues found with checkpatch.pl
> 
> This goes overboard on shortening the commit message line length.  Maybe
> you can configure your editor to do the appropriate line break, or at
> least show the column number.
> 
> >
> > Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
> > ---
> >  drivers/staging/rtl8723bs/os_dep/xmit_linux.c | 15 +++++----------
> >  1 file changed, 5 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> > index f29e110..4861c80 100644
> > --- a/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> > +++ b/drivers/staging/rtl8723bs/os_dep/xmit_linux.c
> > @@ -64,9 +64,7 @@ int rtw_os_xmit_resource_alloc(struct adapter *padapter, struct xmit_buf *pxmitb
> >  	if (alloc_sz > 0) {
> >  		pxmitbuf->pallocated_buf = rtw_zmalloc(alloc_sz);
> >  		if (pxmitbuf->pallocated_buf == NULL)
> > -		{
> >  			return _FAIL;
> > -		}
> 
> OK
> 
> >
> >  		pxmitbuf->pbuf = (u8 *)N_BYTE_ALIGMENT((SIZE_PTR)(pxmitbuf->pallocated_buf), XMITBUF_ALIGN_SZ);
> >  	}
> > @@ -90,8 +88,8 @@ void rtw_os_pkt_complete(struct adapter *padapter, _pkt *pkt)
> >  	queue = skb_get_queue_mapping(pkt);
> >  	if (padapter->registrypriv.wifi_spec) {
> >  		if (__netif_subqueue_stopped(padapter->pnetdev, queue) &&
> > -			(pxmitpriv->hwxmits[queue].accnt < WMM_XMIT_THRESHOLD))
> > -		{
> > +			(pxmitpriv->hwxmits[queue].accnt <
> > +			 WMM_XMIT_THRESHOLD)) {
> 
> This does not look so nice.  What happens if you move the condition to the
> left, such that it starts just to the right of the ( on the preceeding
> line.  If needed, you can also drop the parentheses.
> 
> >  			netif_wake_subqueue(padapter->pnetdev, queue);
> >  		}
> >  	} else {
> > @@ -177,8 +175,7 @@ static int rtw_mlcst2unicst(struct adapter *padapter, struct sk_buff *skb)
> >
> >  	for (i = 0; i < chk_alive_num; i++) {
> >  		psta = rtw_get_stainfo_by_offset(pstapriv, chk_alive_list[i]);
> > -		if (!(psta->state & _FW_LINKED))
> > -		{
> > +		if (!(psta->state & _FW_LINKED)) {
> 
> OK
> 
> >  			DBG_COUNTER(padapter->tx_logs.os_tx_m2u_ignore_fw_linked);
> >  			continue;
> >  		}
> > @@ -187,8 +184,7 @@ static int rtw_mlcst2unicst(struct adapter *padapter, struct sk_buff *skb)
> >  		if (!memcmp(psta->hwaddr, &skb->data[6], 6)
> >  			|| !memcmp(psta->hwaddr, null_addr, 6)
> >  			|| !memcmp(psta->hwaddr, bc_addr, 6)
> > -		)
> > -		{
> > +		) {
> 
> The close parenthesis on the line with the { is not nice.  Move the whole
> thing back to just after the last memcmp call.
Hi, Solving the issue here as you asked raises a "CHECK: Logical continuations should be on the previous line"
It apperntly relates to the '||' being in the start of each line instead of the end.
Moving the '||' to the end solves all issues. Should I add it to the patch ?
> 
> >  			DBG_COUNTER(padapter->tx_logs.os_tx_m2u_ignore_self);
> >  			continue;
> >  		}
> > @@ -249,8 +245,7 @@ int _rtw_xmit_entry(_pkt *pkt, _nic_hdl pnetdev)
> >  			#endif
> >  			)
> >  		&& (padapter->registrypriv.wifi_spec == 0)
> > -		)
> > -	{
> > +		) {
> 
> Same here.
See the longer context of this code https://elixir.bootlin.com/linux/v4.16-rc1/source/drivers/staging/rtl8723bs/os_dep/xmit_linux.c#L248
Here is the same thing as in previous case, the '||' are in the start. It's also not possible to move them to the end
of previous line because it is withing an #if/#endif clause.
> 
> julia
> 
> >  		if (pxmitpriv->free_xmitframe_cnt > (NR_XMITFRAME/4)) {
> >  			res = rtw_mlcst2unicst(padapter, pkt);
> >  			if (res == true) {
> > --
> > 2.7.4
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20180217171924.GA4155%40gmail.com.
> > For more options, visit https://groups.google.com/d/optout.
> >