Re: [Outreachy kernel] Re: [PATCH 1/3] Staging: speakup: Avoid multiple assignments.

[Samuel Thibault <samuel.thibault@ens-lyon.org>]

Sam Muhammed, le lun. 23 mars 2020 19:13:16 -0400, a ecrit:
> On Mon, 2020-03-23 at 23:13 +0100, Samuel Thibault wrote:
> > Hello,
> > 
> > Sam Muhammed, le lun. 23 mars 2020 17:38:43 -0400, a ecrit:
> > > Modify the code to avoid multiple assignments by just
> > > doing the assignment in two steps.
> > 
> > The original version of the code is considered easier to read.
> > Yes, the tools say multiple assignments should be avoided, but that's
> > not a strict rule :)
> 
> Well yes! this got me a bit hesitant, but what got me encouraged to
> make this change is that i looked for patches for the same change in
> this driver and i found this commit:
> 
> 	f23519843b01fdc266dd70ca29ef0f0a54f8b064
> 
> which was applied for the same file main.c, so i thought this driver
> adopted this style, maintaining a common practice?

This is a quite different beast:

-               value = spk_lastkey = pad_chars[value];
+               value = pad_chars[value];
+               spk_lastkey = value;

there is not much in common between "value" and "spk_lastkey", and much
more in common between spk_lastkey and the other spk_* assignments
below.


> > > -	spk_x = spk_cx = vc->vc_x;
> > > -	spk_y = spk_cy = vc->vc_y;
> > > -	spk_pos = spk_cp = vc->vc_pos;
> > > +	spk_cx = vc->vc_x;
> > > +	spk_x = spk_cx;
> > > +	spk_cy = vc->vc_y;
> > > +	spk_y = spk_cy;
> > > +	spk_cp = vc->vc_pos;
> > > +	spk_pos = spk_cp;

Here, there is a lot in common between spk_x and spk_cx, the current
form makes it obvious that we are setting two x variables to vc_x. The
proposed form make its much less clear.

> should i revert this change and drop it from the patchset?

Please drop patch 1 from the 3 yes.

Samuel