Re: [Outreachy kernel] [Outreachy] [PATCH v2 2/3] staging: rtl8188eu: Line over 80 characters

[Soumyajit Deb <debsoumyajit100@gmail.com>]

On Thu, Apr 02, 2020 at 03:04:56PM +0200, Julia Lawall wrote:
> 
> 
> On Thu, 2 Apr 2020, Soumyajit Deb wrote:
> 
> > On Wed, Apr 01, 2020 at 10:41:51AM +0200, Julia Lawall wrote:
> > >
> > >
> > > On Wed, 1 Apr 2020, Soumyajit Deb wrote:
> > >
> > > > On Wed, Apr 01, 2020 at 04:48:38AM +0200, Stefano Brivio wrote:
> > > > > On Wed,  1 Apr 2020 00:02:00 +0530
> > > > > Soumyajit Deb <debsoumyajit100@gmail.com> wrote:
> > > > >
> > > > > > @@ -445,7 +455,8 @@ static void update_bmc_sta(struct adapter *padapter)
> > > > > >  		tx_ra_bitmap = 0xf;
> > > > > >
> > > > > >  		raid = networktype_to_raid(network_type);
> > > > > > -		init_rate = get_highest_rate_idx(tx_ra_bitmap & 0x0fffffff) & 0x3f;
> > > > > > +		init_rate = get_highest_rate_idx(tx_ra_bitmap & 0x0fffffff) &
> > > > > > +							      0x3f;
> > > > >
> > > > > Hmm, no, wait, what is 0x3f bit-wise intersected with?
> > > > >
> > > > > a = lol;
> > > > >
> > > > > a = lol *
> > > > >     2;
> > > > >
> > > > > a = (lol + foo) *
> > > > >     2;
> > > > >
> > > > > a = lol + foo *
> > > > >           2;
> > > > >
> > > > > but not:
> > > > >
> > > > > a = (lol + foo) *
> > > > >          2;
> > > > >
> > > > > ...right?
> > > > >
> > > >
> > > > I am sorry but I couldn't understand what you mean.
> > > >
> > > > As much I can see, a bitwise AND operation is being done between 0x3f and the
> > > > return value of get_highest_rate_idx() function. The first & operation
> > > > is between the arguments of get_highest_rate_idx() function.
> > >
> > > Putting the 0x3 underneath the middle of the argument list of the left
> > > argument of the & is very strange.  get_highest_rate_idx is going to take
> > > its argument and produce some value.  That value will be &ed with 0x3f.
> > > There is no interaction between the arguments of this function and 0x3f,
> > > so it is misleading to try to associate them in some way.  Lining up the
> > > left side of get_highest_rate_idx (left argument of &) with the left side
> > > of 0x3f (right argument of &) could be fine.
> > >
> > > >
> > > > If the current alignment is wrong, should I place 0x3f someplace else?
> > > >
> > > > But as you suggested previously in this thread, that I should place 0x3f
> > > > below the start of first & operand.
> > >
> > > There may have been a confusion because there are two &s in this code.
> > >
> > > julia
> > >
> > Hii Julia,
> >
> > Okay, understood.
> > Then should I place the 0x3f on the left end of the function argument
> > of get_highest_rate_idx() function and resend this patch series?
> 
> No, not the argument.  At the left end of the function call.  The & has
> two arguments; the function call on the left and the 0x3f on the right.
> 
> julia
>

Okay, understood.

My bad, for a minute I got confused and started to consider 0x3f as a
part of the argument. 

I will make the required changes and resend the v3 of the patch
series.

Thank you

--
Soumyajit


> >
> > Thank you
> >
> > --
> > Soumyajit
> >
> > > >
> > > > > >
> > > > > >  		/* ap mode */
> > > > > >  		rtw_hal_set_odm_var(padapter, HAL_ODM_STA_INFO, psta, true);
> > > > > > @@ -456,7 +467,8 @@ static void update_bmc_sta(struct adapter *padapter)
> > > > > >  			arg = psta->mac_id & 0x1f;
> > > > > >  			arg |= BIT(7);
> > > > > >  			tx_ra_bitmap |= ((raid << 28) & 0xf0000000);
> > > > > > -			DBG_88E("%s, mask = 0x%x, arg = 0x%x\n", __func__, tx_ra_bitmap, arg);
> > > > > > +			DBG_88E("%s, mask = 0x%x, arg = 0x%x\n", __func__,
> > > > > > +				tx_ra_bitmap, arg);
> > > > > >
> > > > > >  			/* bitmap[0:27] = tx_rate_bitmap */
> > > > > >  			/* bitmap[28:31]= Rate Adaptive id */
> > > > > > @@ -647,7 +659,8 @@ static void start_bss_network(struct adapter *padapter, u8 *pbuf)
> > > > > >  	rtw_hal_set_hwreg(padapter, HW_VAR_SEC_CFG, (u8 *)(&val8));
> > > > > >
> > > > > >  	/* Beacon Control related register */
> > > > > > -	rtw_hal_set_hwreg(padapter, HW_VAR_BEACON_INTERVAL, (u8 *)(&bcn_interval));
> > > > > > +	rtw_hal_set_hwreg(padapter, HW_VAR_BEACON_INTERVAL,
> > > > > > +			  (u8 *)(&bcn_interval));
> > > > >
> > > > > This is another pair of parentheses that you could remove: &a == (&a).
> > > > >
> > > > > --
> > > > > Stefano
> > > >
> > > > I noticed those parentheses while removing other extra parentheses
> > > > but I didn't remove these as most of these parentheses are present
> > > > after a type cast.
> > > > Will it not look confusing if I remove these paretheses?
> > > > If not, I will remove them :)
> > > >
> > > > Thank you.
> > > >
> > > > --
> > > > Soumyajit
> > > > >
> > > >
> > > > --
> > > > 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 view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20200401075902.GA18430%40ubuntu.
> > > >
> >
> > --
> > 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 view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20200402130127.GA9847%40ubuntu.
> >