[PATCH] staging: vt6655: device_main: switch multiple assignment for one assignment per line

[PATCH] staging: vt6655: device_main: switch multiple assignment for one assignment per line

[Lourdes Pedrajas]

One assignment per line is preferred, instead of multiple assignments.
To prevent possible undefined behavior.
Issue found with checkpath.

Signed-off-by: Lourdes Pedrajas <lu@pplo.net>
---
 drivers/staging/vt6655/device_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index 0442f71494b2..c271e3937125 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -708,7 +708,8 @@ static int device_init_td1_ring(struct vnt_private *priv)
 
 	if (i > 0)
 		priv->apTD1Rings[i - 1].next_desc = cpu_to_le32(priv->td1_pool_dma);
-	priv->apTailTD[1] = priv->apCurrTD[1] = &priv->apTD1Rings[0];
+	priv->apCurrTD[1] = &priv->apTD1Rings[0];
+	priv->apTailTD[1] = &priv->apTD1Rings[0];
 
 	return 0;
 
-- 
2.17.1

Re: [Outreachy kernel] [PATCH] staging: vt6655: device_main: switch multiple assignment for one assignment per line

[Julia Lawall]

On Sun, 15 Mar 2020, Lourdes Pedrajas wrote:

> One assignment per line is preferred, instead of multiple assignments.
> To prevent possible undefined behavior.

There is no possible undefined behavior.  The C semantics provides a
definition of what this means.  On the other hand, people don't seem to be
so familiar with what this semantics is.  Looking quickly with Google, I
find some pages that think that it is the value of the left side
expression and some pages that this that it is the value of the right side
expression.  This can have an impact if the types are different and the
assignment involves an implicit cast.  Here all the types are the same.

I'm not really sure which would be preferred.  The original code is a bit
more concise.  The modified code is a bit more clear, because one can find
what is assigned by just looking in one standard place, ie the left side
of the code.

> Issue found with checkpath.

checkpatch :)

julia

>
> Signed-off-by: Lourdes Pedrajas <lu@pplo.net>
> ---
>  drivers/staging/vt6655/device_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> index 0442f71494b2..c271e3937125 100644
> --- a/drivers/staging/vt6655/device_main.c
> +++ b/drivers/staging/vt6655/device_main.c
> @@ -708,7 +708,8 @@ static int device_init_td1_ring(struct vnt_private *priv)
>
>  	if (i > 0)
>  		priv->apTD1Rings[i - 1].next_desc = cpu_to_le32(priv->td1_pool_dma);
> -	priv->apTailTD[1] = priv->apCurrTD[1] = &priv->apTD1Rings[0];
> +	priv->apCurrTD[1] = &priv->apTD1Rings[0];
> +	priv->apTailTD[1] = &priv->apTD1Rings[0];
>
>  	return 0;
>
> --
> 2.17.1
>
> --
> 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/20200315044620.14104-1-lu%40pplo.net.
>

Re: [Outreachy kernel] [PATCH] staging: vt6655: device_main: switch multiple assignment for one assignment per line

[Lourdes Pedrajas]

On Sun, Mar 15, 2020 at 09:05:02AM +0100, Julia Lawall wrote:
> 
> 
> On Sun, 15 Mar 2020, Lourdes Pedrajas wrote:
> 
> > One assignment per line is preferred, instead of multiple assignments.
> > To prevent possible undefined behavior.
> 
> There is no possible undefined behavior.  The C semantics provides a
> definition of what this means.  On the other hand, people don't seem to be
> so familiar with what this semantics is.  Looking quickly with Google, I
> find some pages that think that it is the value of the left side
> expression and some pages that this that it is the value of the right side
> expression.  This can have an impact if the types are different and the
> assignment involves an implicit cast.  Here all the types are the same.
> 
> I'm not really sure which would be preferred.  The original code is a bit
> more concise.  The modified code is a bit more clear, because one can find
> what is assigned by just looking in one standard place, ie the left side
> of the code.
> 
> > Issue found with checkpath.
> 
> checkpatch :)
> 
> julia
>

I also was unsure if it was undefined behavior as checkpatch says, it
assign 3rd value to 2nd, and then to 1st, to me is clear.
The change may make the code more readable and would help people that is not 
familiar with multiple assignments.

If the maintainer agree, I will simply correct the typo in checkpatch :)

Thank you Julia!

Lourdes

Re: [PATCH] staging: vt6655: device_main: switch multiple assignment for one assignment per line

[Greg KH]

On Sun, Mar 15, 2020 at 05:46:20AM +0100, Lourdes Pedrajas wrote:
> One assignment per line is preferred, instead of multiple assignments.
> To prevent possible undefined behavior.
> Issue found with checkpath.
> 
> Signed-off-by: Lourdes Pedrajas <lu@pplo.net>
> ---
>  drivers/staging/vt6655/device_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> index 0442f71494b2..c271e3937125 100644
> --- a/drivers/staging/vt6655/device_main.c
> +++ b/drivers/staging/vt6655/device_main.c
> @@ -708,7 +708,8 @@ static int device_init_td1_ring(struct vnt_private *priv)
>  
>  	if (i > 0)
>  		priv->apTD1Rings[i - 1].next_desc = cpu_to_le32(priv->td1_pool_dma);
> -	priv->apTailTD[1] = priv->apCurrTD[1] = &priv->apTD1Rings[0];
> +	priv->apCurrTD[1] = &priv->apTD1Rings[0];
> +	priv->apTailTD[1] = &priv->apTD1Rings[0];

As others said here, there is no undefined behavior, the original code
is just fine.

thanks,

greg k-h

Re: [PATCH] staging: vt6655: device_main: switch multiple assignment for one assignment per line

[Lourdes Pedrajas]

On Tue, Mar 17, 2020 at 12:09:38PM +0100, Greg KH wrote:
> On Sun, Mar 15, 2020 at 05:46:20AM +0100, Lourdes Pedrajas wrote:
> > One assignment per line is preferred, instead of multiple assignments.
> > To prevent possible undefined behavior.
> > Issue found with checkpath.
> > 
> > Signed-off-by: Lourdes Pedrajas <lu@pplo.net>
> > ---
> >  drivers/staging/vt6655/device_main.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> > index 0442f71494b2..c271e3937125 100644
> > --- a/drivers/staging/vt6655/device_main.c
> > +++ b/drivers/staging/vt6655/device_main.c
> > @@ -708,7 +708,8 @@ static int device_init_td1_ring(struct vnt_private *priv)
> >  
> >  	if (i > 0)
> >  		priv->apTD1Rings[i - 1].next_desc = cpu_to_le32(priv->td1_pool_dma);
> > -	priv->apTailTD[1] = priv->apCurrTD[1] = &priv->apTD1Rings[0];
> > +	priv->apCurrTD[1] = &priv->apTD1Rings[0];
> > +	priv->apTailTD[1] = &priv->apTD1Rings[0];
> 
> As others said here, there is no undefined behavior, the original code
> is just fine.
> 
> thanks,
> 
> greg k-h

I agree, thank you,

Lourdes