Re: [PATCH 1/4 v2] silicom: checkpatch: assignments in if conditions

[Dan Carpenter <dan.carpenter@oracle.com>]

On Mon, Jun 17, 2013 at 09:15:39PM +0200, Lorenz Haspel wrote:
> Fixes checkpatch error:
> There were assignments in if conditions, so I extracted them.
> 
> Signed-off-by: Lorenz Haspel <lorenz@badgers.com>
> Signed-off-by: Michael Banken <michael.banken@mathe.stud.uni-erlangen.de>
> ---
> v2: removed some buggy extra lines and fixed white space issues

Gar....  This isn't right either.  Now it has *too many* blank
lines.  It's only between declarations and code that I was
complaining about.  You've added them between assignments and error
checks.


> @@ -1224,7 +1237,9 @@ static int wdt_pulse(bpctl_dev_t *pbpctl_dev)
>  		return -1;
>  #endif
>  	if (pbpctl_dev->bp_10g9) {
> -		if (!(pbpctl_dev_c = get_status_port_fn(pbpctl_dev)))
> +		pbpctl_dev_c = get_status_port_fn(pbpctl_dev);
> +

This blank line is harmful.

> +		if (!pbpctl_dev_c)
>  			return -1;
>  	}
>  
> @@ -1742,9 +1757,9 @@ static void write_data_port_int(bpctl_dev_t *pbpctl_dev,
>  
>  static int write_data_int(bpctl_dev_t *pbpctl_dev, unsigned char value)
>  {
> -	bpctl_dev_t *pbpctl_dev_b = NULL;
> +	bpctl_dev_t *pbpctl_dev_b = get_status_port_fn(pbpctl_dev);
>  

This blank line is required.

So what you have here is fine, but if you wanted you could re-write
this like:
{
	bpctl_dev_t *pbpctl_dev_b;

	pbpctl_dev_b = get_status_port_fn(pbpctl_dev);
	if (!pbpctl_dev_b)
		return -1;

Generally, you shouldn't put anything complicated in the initializer
statement.  People don't read that code as thouroughly and
initializers are sometimes a source of bugs.  But what you have here
is also perfectly acceptable.

> -	if (!(pbpctl_dev_b = get_status_port_fn(pbpctl_dev)))
> +	if (!pbpctl_dev_b)
>  		return -1;
>  	atomic_set(&pbpctl_dev->wdt_busy, 1);
>  	write_data_port_int(pbpctl_dev, value & 0x3);

rergards,
dan carpenter