[PATCH 1/2] Align member-assigns in a structure-copy block

[PATCH 1/2] Align member-assigns in a structure-copy block

[Giel van Schijndel]

This highlights the differences (errors).
---
 drivers/net/wireless/ti/wlcore/acx.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
index b924cea..beb354c 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -1715,17 +1715,17 @@ int wl12xx_acx_config_hangover(struct wl1271 *wl)
 		goto out;
 	}
 
-	acx->recover_time = cpu_to_le32(conf->recover_time);
-	acx->hangover_period = conf->hangover_period;
-	acx->dynamic_mode = conf->dynamic_mode;
-	acx->early_termination_mode = conf->early_termination_mode;
-	acx->max_period = conf->max_period;
-	acx->min_period = conf->min_period;
-	acx->increase_delta = conf->increase_delta;
-	acx->decrease_delta = conf->decrease_delta;
-	acx->quiet_time = conf->quiet_time;
-	acx->increase_time = conf->increase_time;
-	acx->window_size = acx->window_size;
+	acx->recover_time               = cpu_to_le32(conf->recover_time);
+	acx->hangover_period            = conf->hangover_period;
+	acx->dynamic_mode               = conf->dynamic_mode;
+	acx->early_termination_mode     = conf->early_termination_mode;
+	acx->max_period                 = conf->max_period;
+	acx->min_period                 = conf->min_period;
+	acx->increase_delta             = conf->increase_delta;
+	acx->decrease_delta             = conf->decrease_delta;
+	acx->quiet_time                 = conf->quiet_time;
+	acx->increase_time              = conf->increase_time;
+	acx->window_size                = acx->window_size;
 
 	ret = wl1271_cmd_configure(wl, ACX_CONFIG_HANGOVER, acx,
 				   sizeof(*acx));
-- 
2.1.4

[PATCH 2/2] Fix copy-paste bug: assign from src struct not dest

[Giel van Schijndel]

---
 drivers/net/wireless/ti/wlcore/acx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
index beb354c..93a2fa8 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -1725,7 +1725,7 @@ int wl12xx_acx_config_hangover(struct wl1271 *wl)
 	acx->decrease_delta             = conf->decrease_delta;
 	acx->quiet_time                 = conf->quiet_time;
 	acx->increase_time              = conf->increase_time;
-	acx->window_size                = acx->window_size;
+	acx->window_size                = conf->window_size;
 
 	ret = wl1271_cmd_configure(wl, ACX_CONFIG_HANGOVER, acx,
 				   sizeof(*acx));
-- 
2.1.4

Re: [PATCH 2/2] Fix copy-paste bug: assign from src struct not dest

[Giel van Schijndel]

[-- Attachment #1: Type: text/plain, Size: 210 bytes --]

On Sun, Jan 04, 2015 at 19:00:23 +0100, Giel van Schijndel wrote:
> ---

Forgot to:
Signed-off-by: Giel van Schijndel <me@mortis.eu>

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/2] Fix copy-paste bug: assign from src struct not dest

[Johannes Berg]

On Sun, 2015-01-04 at 19:00 +0100, Giel van Schijndel wrote:
> ---
>  drivers/net/wireless/ti/wlcore/acx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
> index beb354c..93a2fa8 100644
> --- a/drivers/net/wireless/ti/wlcore/acx.c
> +++ b/drivers/net/wireless/ti/wlcore/acx.c
> @@ -1725,7 +1725,7 @@ int wl12xx_acx_config_hangover(struct wl1271 *wl)
>  	acx->decrease_delta             = conf->decrease_delta;
>  	acx->quiet_time                 = conf->quiet_time;
>  	acx->increase_time              = conf->increase_time;
> -	acx->window_size                = acx->window_size;
> +	acx->window_size                = conf->window_size;

It would be far better to fix the bug *first*, that way the bugfix can
be cherry-picked/applied to trees that don't have the alignment.

(And anyway I question the value of the alignment - if you really want
to make this bug disappear you could perhaps use a macro)

johannes

Re: [PATCH 2/2] Fix copy-paste bug: assign from src struct not dest

[Giel van Schijndel]

[-- Attachment #1: Type: text/plain, Size: 1949 bytes --]

On Mon, Jan 05, 2015 at 10:54:31 +0100, Johannes Berg wrote:
> On Sun, 2015-01-04 at 19:00 +0100, Giel van Schijndel wrote:
>> ---
>>  drivers/net/wireless/ti/wlcore/acx.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
>> index beb354c..93a2fa8 100644
>> --- a/drivers/net/wireless/ti/wlcore/acx.c
>> +++ b/drivers/net/wireless/ti/wlcore/acx.c
>> @@ -1725,7 +1725,7 @@ int wl12xx_acx_config_hangover(struct wl1271 *wl)
>>  	acx->decrease_delta             = conf->decrease_delta;
>>  	acx->quiet_time                 = conf->quiet_time;
>>  	acx->increase_time              = conf->increase_time;
>> -	acx->window_size                = acx->window_size;
>> +	acx->window_size                = conf->window_size;
> 
> It would be far better to fix the bug *first*, that way the bugfix can
> be cherry-picked/applied to trees that don't have the alignment.

I agree on the ordering.

As for:
> (And anyway I question the value of the alignment - if you really want
> to make this bug disappear you could perhaps use a macro)

And:
On Wed, Jan 07, 2015 at 20:40:57 +0200, Andy Shevchenko wrote:
> On Sun, Jan 4, 2015 at 8:00 PM, Giel van Schijndel <me@mortis.eu> wrote:
> > This highlights the differences (errors).
> 
> Seems like patch for the patch. Just fix an error like it's done here:
> http://www.spinics.net/lists/linux-wireless/msg131667.html

IMO the aligned block of code has the significant advantage of taking
advantage of humans' ability to spot things that break a pattern. Which
in this case becomes *very* visible when properly aligned, because
without the alignment there is no (visual) pattern (or at least not one
very suitable for my "visual processing system", I know the same applies
to at least some others).

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/2] Fix copy-paste bug: assign from src struct not dest

[Johannes Berg]

On Wed, 2015-01-07 at 20:18 +0100, Giel van Schijndel wrote:

> IMO the aligned block of code has the significant advantage of taking
> advantage of humans' ability to spot things that break a pattern. Which
> in this case becomes *very* visible when properly aligned, because
> without the alignment there is no (visual) pattern (or at least not one
> very suitable for my "visual processing system", I know the same applies
> to at least some others).

Yeah, well, but why even invoke that "visual processing system"?

If you look, for example, at the __skb_clone function it just uses a
macro:

#define C(x) n->x = skb->x

and then

	C(len);
	C(data_len);

etc.

johannes

Re: [PATCH 2/2] Fix copy-paste bug: assign from src struct not dest

[Arend van Spriel]

On 01/07/15 23:16, Johannes Berg wrote:
> On Wed, 2015-01-07 at 20:18 +0100, Giel van Schijndel wrote:
>
>> IMO the aligned block of code has the significant advantage of taking
>> advantage of humans' ability to spot things that break a pattern. Which
>> in this case becomes *very* visible when properly aligned, because
>> without the alignment there is no (visual) pattern (or at least not one
>> very suitable for my "visual processing system", I know the same applies
>> to at least some others).
>
> Yeah, well, but why even invoke that "visual processing system"?
>
> If you look, for example, at the __skb_clone function it just uses a
> macro:
>
> #define C(x) n->x = skb->x

This requires fixed names so I generally prefer to add them:

#define C(d, s, f)	(d)->f = (s)->f

> and then
>
> 	C(len);
> 	C(data_len);

	C(acx, conf, window_size);
	C(acx, conf, increase_time);

Regards,
Arend

>
> etc.
>
> johannes
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] Align member-assigns in a structure-copy block

[Giel van Schijndel]

[-- Attachment #1: Type: text/plain, Size: 377 bytes --]

On Sun, Jan 04, 2015 at 19:00:22 +0100, Giel van Schijndel wrote:
> This highlights the differences (errors).
> ---

Forgot to:
Signed-off-by: Giel van Schijndel <me@mortis.eu>

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"Walking on water and developing software from a specification are easy
 if both are frozen."
  -- Edward V Berard

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/2] Align member-assigns in a structure-copy block

[Kalle Valo]

Giel van Schijndel <me@mortis.eu> writes:

> On Sun, Jan 04, 2015 at 19:00:22 +0100, Giel van Schijndel wrote:
>> This highlights the differences (errors).
>> ---
>
> Forgot to:
> Signed-off-by: Giel van Schijndel <me@mortis.eu>

I don't normally edit patches (don't have time for that), so please
resend.

-- 
Kalle Valo

Re: [PATCH 1/2] Align member-assigns in a structure-copy block

[Kalle Valo]

Giel van Schijndel <me@mortis.eu> writes:

> This highlights the differences (errors).
> ---
>  drivers/net/wireless/ti/wlcore/acx.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)

Please prefix the patch titles with "wlcore: ".

-- 
Kalle Valo

Re: [PATCH 1/2] Align member-assigns in a structure-copy block

[Andy Shevchenko]

On Sun, Jan 4, 2015 at 8:00 PM, Giel van Schijndel <me-sZ9Uef1cvPWHXe+LvDLADg@public.gmane.org> wrote:
> This highlights the differences (errors).

Seems like patch for the patch. Just fix an error like it's done here:
http://www.spinics.net/lists/linux-wireless/msg131667.html


> ---
>  drivers/net/wireless/ti/wlcore/acx.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
> index b924cea..beb354c 100644
> --- a/drivers/net/wireless/ti/wlcore/acx.c
> +++ b/drivers/net/wireless/ti/wlcore/acx.c
> @@ -1715,17 +1715,17 @@ int wl12xx_acx_config_hangover(struct wl1271 *wl)
>                 goto out;
>         }
>
> -       acx->recover_time = cpu_to_le32(conf->recover_time);
> -       acx->hangover_period = conf->hangover_period;
> -       acx->dynamic_mode = conf->dynamic_mode;
> -       acx->early_termination_mode = conf->early_termination_mode;
> -       acx->max_period = conf->max_period;
> -       acx->min_period = conf->min_period;
> -       acx->increase_delta = conf->increase_delta;
> -       acx->decrease_delta = conf->decrease_delta;
> -       acx->quiet_time = conf->quiet_time;
> -       acx->increase_time = conf->increase_time;
> -       acx->window_size = acx->window_size;
> +       acx->recover_time               = cpu_to_le32(conf->recover_time);
> +       acx->hangover_period            = conf->hangover_period;
> +       acx->dynamic_mode               = conf->dynamic_mode;
> +       acx->early_termination_mode     = conf->early_termination_mode;
> +       acx->max_period                 = conf->max_period;
> +       acx->min_period                 = conf->min_period;
> +       acx->increase_delta             = conf->increase_delta;
> +       acx->decrease_delta             = conf->decrease_delta;
> +       acx->quiet_time                 = conf->quiet_time;
> +       acx->increase_time              = conf->increase_time;
> +       acx->window_size                = acx->window_size;
>
>         ret = wl1271_cmd_configure(wl, ACX_CONFIG_HANGOVER, acx,
>                                    sizeof(*acx));
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND 1/2] wlcore: fix copy-paste bug: assign from src struct not dest

[Giel van Schijndel]

Signed-off-by: Giel van Schijndel <me@mortis.eu>
Reported-at: http://www.viva64.com/en/b/0299/
---
 drivers/net/wireless/ti/wlcore/acx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
index b924cea..f28fa3b 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -1725,7 +1725,7 @@ int wl12xx_acx_config_hangover(struct wl1271 *wl)
 	acx->decrease_delta = conf->decrease_delta;
 	acx->quiet_time = conf->quiet_time;
 	acx->increase_time = conf->increase_time;
-	acx->window_size = acx->window_size;
+	acx->window_size = conf->window_size;
 
 	ret = wl1271_cmd_configure(wl, ACX_CONFIG_HANGOVER, acx,
 				   sizeof(*acx));
-- 
2.1.4

[PATCH RESEND 2/2] wlcore: align member-assigns in a structure-copy block

[Giel van Schijndel]

This highlights the differences (e.g. the bug fixed in the previous
commit).

Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
 drivers/net/wireless/ti/wlcore/acx.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
index f28fa3b..93a2fa8 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -1715,17 +1715,17 @@ int wl12xx_acx_config_hangover(struct wl1271 *wl)
 		goto out;
 	}
 
-	acx->recover_time = cpu_to_le32(conf->recover_time);
-	acx->hangover_period = conf->hangover_period;
-	acx->dynamic_mode = conf->dynamic_mode;
-	acx->early_termination_mode = conf->early_termination_mode;
-	acx->max_period = conf->max_period;
-	acx->min_period = conf->min_period;
-	acx->increase_delta = conf->increase_delta;
-	acx->decrease_delta = conf->decrease_delta;
-	acx->quiet_time = conf->quiet_time;
-	acx->increase_time = conf->increase_time;
-	acx->window_size = conf->window_size;
+	acx->recover_time               = cpu_to_le32(conf->recover_time);
+	acx->hangover_period            = conf->hangover_period;
+	acx->dynamic_mode               = conf->dynamic_mode;
+	acx->early_termination_mode     = conf->early_termination_mode;
+	acx->max_period                 = conf->max_period;
+	acx->min_period                 = conf->min_period;
+	acx->increase_delta             = conf->increase_delta;
+	acx->decrease_delta             = conf->decrease_delta;
+	acx->quiet_time                 = conf->quiet_time;
+	acx->increase_time              = conf->increase_time;
+	acx->window_size                = conf->window_size;
 
 	ret = wl1271_cmd_configure(wl, ACX_CONFIG_HANGOVER, acx,
 				   sizeof(*acx));
-- 
2.1.4

Re: [PATCH RESEND 2/2] wlcore: align member-assigns in a structure-copy block

[Kalle Valo]

Giel van Schijndel <me-sZ9Uef1cvPWHXe+LvDLADg@public.gmane.org> writes:

> This highlights the differences (e.g. the bug fixed in the previous
> commit).
>
> Signed-off-by: Giel van Schijndel <me-sZ9Uef1cvPWHXe+LvDLADg@public.gmane.org>
> ---
>  drivers/net/wireless/ti/wlcore/acx.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
> index f28fa3b..93a2fa8 100644
> --- a/drivers/net/wireless/ti/wlcore/acx.c
> +++ b/drivers/net/wireless/ti/wlcore/acx.c
> @@ -1715,17 +1715,17 @@ int wl12xx_acx_config_hangover(struct wl1271 *wl)
>  		goto out;
>  	}
>  
> -	acx->recover_time = cpu_to_le32(conf->recover_time);
> -	acx->hangover_period = conf->hangover_period;
> -	acx->dynamic_mode = conf->dynamic_mode;
> -	acx->early_termination_mode = conf->early_termination_mode;
> -	acx->max_period = conf->max_period;
> -	acx->min_period = conf->min_period;
> -	acx->increase_delta = conf->increase_delta;
> -	acx->decrease_delta = conf->decrease_delta;
> -	acx->quiet_time = conf->quiet_time;
> -	acx->increase_time = conf->increase_time;
> -	acx->window_size = conf->window_size;
> +	acx->recover_time               = cpu_to_le32(conf->recover_time);
> +	acx->hangover_period            = conf->hangover_period;
> +	acx->dynamic_mode               = conf->dynamic_mode;
> +	acx->early_termination_mode     = conf->early_termination_mode;
> +	acx->max_period                 = conf->max_period;
> +	acx->min_period                 = conf->min_period;
> +	acx->increase_delta             = conf->increase_delta;
> +	acx->decrease_delta             = conf->decrease_delta;
> +	acx->quiet_time                 = conf->quiet_time;
> +	acx->increase_time              = conf->increase_time;
> +	acx->window_size                = conf->window_size;

I would like to get an ACK from one of the wlcore developers if I should
apply this (or not).

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND 2/2] wlcore: align member-assigns in a structure-copy block

[Eliad Peller]

On Fri, Jan 9, 2015 at 7:03 PM, Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> Giel van Schijndel <me-sZ9Uef1cvPWHXe+LvDLADg@public.gmane.org> writes:
>
>> This highlights the differences (e.g. the bug fixed in the previous
>> commit).
>>
>> Signed-off-by: Giel van Schijndel <me-sZ9Uef1cvPWHXe+LvDLADg@public.gmane.org>
>> ---
>>  drivers/net/wireless/ti/wlcore/acx.c | 22 +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
>> index f28fa3b..93a2fa8 100644
>> --- a/drivers/net/wireless/ti/wlcore/acx.c
>> +++ b/drivers/net/wireless/ti/wlcore/acx.c
>> @@ -1715,17 +1715,17 @@ int wl12xx_acx_config_hangover(struct wl1271 *wl)
>>               goto out;
>>       }
>>
>> -     acx->recover_time = cpu_to_le32(conf->recover_time);
>> -     acx->hangover_period = conf->hangover_period;
>> -     acx->dynamic_mode = conf->dynamic_mode;
>> -     acx->early_termination_mode = conf->early_termination_mode;
>> -     acx->max_period = conf->max_period;
>> -     acx->min_period = conf->min_period;
>> -     acx->increase_delta = conf->increase_delta;
>> -     acx->decrease_delta = conf->decrease_delta;
>> -     acx->quiet_time = conf->quiet_time;
>> -     acx->increase_time = conf->increase_time;
>> -     acx->window_size = conf->window_size;
>> +     acx->recover_time               = cpu_to_le32(conf->recover_time);
>> +     acx->hangover_period            = conf->hangover_period;
>> +     acx->dynamic_mode               = conf->dynamic_mode;
>> +     acx->early_termination_mode     = conf->early_termination_mode;
>> +     acx->max_period                 = conf->max_period;
>> +     acx->min_period                 = conf->min_period;
>> +     acx->increase_delta             = conf->increase_delta;
>> +     acx->decrease_delta             = conf->decrease_delta;
>> +     acx->quiet_time                 = conf->quiet_time;
>> +     acx->increase_time              = conf->increase_time;
>> +     acx->window_size                = conf->window_size;
>
> I would like to get an ACK from one of the wlcore developers if I should
> apply this (or not).
>
I don't have a strong opinion here.
However, it looks pretty much redundant to take a random blob (which
was just fixed by a correct patch) and re-indent it.
The rest of the file doesn't follow this style, so i don't see a good
reason to apply it here.

I agree such indentation have some benefit, but it won't help with the
more common use case (of copy-paste error) of copying the wrong field
(i.e. D->a = S->b instead of D->a = S->a).
For these cases the macros suggested by Arend and Johannes will do the
trick. However i usually dislike such macros, as they tend to break
some IDE features (e.g. auto completion).
Maybe we can come up with some nice spatch to catch these cases.

Just my 2c.

Eliad.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND 2/2] wlcore: align member-assigns in a structure-copy block

[Giel van Schijndel]

[-- Attachment #1: Type: text/plain, Size: 3868 bytes --]

On Sun, Jan 11, 2015 at 12:22:49 +0200, Eliad Peller wrote:
> On Fri, Jan 9, 2015 at 7:03 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>> Giel van Schijndel <me@mortis.eu> writes:
>>> This highlights the differences (e.g. the bug fixed in the previous
>>> commit).
>>>
>>> Signed-off-by: Giel van Schijndel <me@mortis.eu>
>>> ---
>>>  drivers/net/wireless/ti/wlcore/acx.c | 22 +++++++++++-----------
>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
>>> index f28fa3b..93a2fa8 100644
>>> --- a/drivers/net/wireless/ti/wlcore/acx.c
>>> +++ b/drivers/net/wireless/ti/wlcore/acx.c
>>> @@ -1715,17 +1715,17 @@ int wl12xx_acx_config_hangover(struct wl1271 *wl)
>>>               goto out;
>>>       }
>>>
>>> -     acx->recover_time = cpu_to_le32(conf->recover_time);
>>> -     acx->hangover_period = conf->hangover_period;
>>> -     acx->dynamic_mode = conf->dynamic_mode;
>>> -     acx->early_termination_mode = conf->early_termination_mode;
>>> -     acx->max_period = conf->max_period;
>>> -     acx->min_period = conf->min_period;
>>> -     acx->increase_delta = conf->increase_delta;
>>> -     acx->decrease_delta = conf->decrease_delta;
>>> -     acx->quiet_time = conf->quiet_time;
>>> -     acx->increase_time = conf->increase_time;
>>> -     acx->window_size = conf->window_size;
>>> +     acx->recover_time               = cpu_to_le32(conf->recover_time);
>>> +     acx->hangover_period            = conf->hangover_period;
>>> +     acx->dynamic_mode               = conf->dynamic_mode;
>>> +     acx->early_termination_mode     = conf->early_termination_mode;
>>> +     acx->max_period                 = conf->max_period;
>>> +     acx->min_period                 = conf->min_period;
>>> +     acx->increase_delta             = conf->increase_delta;
>>> +     acx->decrease_delta             = conf->decrease_delta;
>>> +     acx->quiet_time                 = conf->quiet_time;
>>> +     acx->increase_time              = conf->increase_time;
>>> +     acx->window_size                = conf->window_size;
>>
>> I would like to get an ACK from one of the wlcore developers if I should
>> apply this (or not).
>>
> I don't have a strong opinion here.
> However, it looks pretty much redundant to take a random blob (which
> was just fixed by a correct patch) and re-indent it.
> The rest of the file doesn't follow this style, so i don't see a good
> reason to apply it here.
> 
> I agree such indentation have some benefit, but it won't help with the
> more common use case (of copy-paste error) of copying the wrong field
> (i.e. D->a = S->b instead of D->a = S->a).
> For these cases the macros suggested by Arend and Johannes will do the
> trick. However i usually dislike such macros, as they tend to break
> some IDE features (e.g. auto completion).
> Maybe we can come up with some nice spatch to catch these cases.

What I dislike about those macros is just that they're not as familiar
to any C programmer as the assignment operator, so they make the code
less readable (even if just a little bit).

As for the IDE thing: I try not to use them, but have been told (by
colleagues) that Eclipse is reasonably smart about macros in C. I use
VIM with the clang_complete plugin and that does do proper completion
with expressions containing macros, but not inside macros based on what
the macro expansion would be, like the one above.

That's why I believe this kind of alignment is at least *an* improvement
even if it doesn't solve all possible problems.

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"Question: what do you call your programming methodology?
 Answer: Faith based development. You code and then pray that it works."
  -- John Spelner

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH RESEND 2/2] wlcore: align member-assigns in a structure-copy block

[Kalle Valo]

Eliad Peller <eliad@wizery.com> writes:

> On Fri, Jan 9, 2015 at 7:03 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>> Giel van Schijndel <me@mortis.eu> writes:
>>
>>> This highlights the differences (e.g. the bug fixed in the previous
>>> commit).
>>>
>>> Signed-off-by: Giel van Schijndel <me@mortis.eu>
>>> ---
>>>  drivers/net/wireless/ti/wlcore/acx.c | 22 +++++++++++-----------
>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
>>> index f28fa3b..93a2fa8 100644
>>> --- a/drivers/net/wireless/ti/wlcore/acx.c
>>> +++ b/drivers/net/wireless/ti/wlcore/acx.c
>>> @@ -1715,17 +1715,17 @@ int wl12xx_acx_config_hangover(struct wl1271 *wl)
>>>               goto out;
>>>       }
>>>
>>> -     acx->recover_time = cpu_to_le32(conf->recover_time);
>>> -     acx->hangover_period = conf->hangover_period;
>>> -     acx->dynamic_mode = conf->dynamic_mode;
>>> -     acx->early_termination_mode = conf->early_termination_mode;
>>> -     acx->max_period = conf->max_period;
>>> -     acx->min_period = conf->min_period;
>>> -     acx->increase_delta = conf->increase_delta;
>>> -     acx->decrease_delta = conf->decrease_delta;
>>> -     acx->quiet_time = conf->quiet_time;
>>> -     acx->increase_time = conf->increase_time;
>>> -     acx->window_size = conf->window_size;
>>> +     acx->recover_time               = cpu_to_le32(conf->recover_time);
>>> +     acx->hangover_period            = conf->hangover_period;
>>> +     acx->dynamic_mode               = conf->dynamic_mode;
>>> +     acx->early_termination_mode     = conf->early_termination_mode;
>>> +     acx->max_period                 = conf->max_period;
>>> +     acx->min_period                 = conf->min_period;
>>> +     acx->increase_delta             = conf->increase_delta;
>>> +     acx->decrease_delta             = conf->decrease_delta;
>>> +     acx->quiet_time                 = conf->quiet_time;
>>> +     acx->increase_time              = conf->increase_time;
>>> +     acx->window_size                = conf->window_size;
>>
>> I would like to get an ACK from one of the wlcore developers if I should
>> apply this (or not).
>
> I don't have a strong opinion here. However, it looks pretty much
> redundant to take a random blob (which was just fixed by a correct
> patch) and re-indent it. The rest of the file doesn't follow this
> style, so i don't see a good reason to apply it here.

Yeah, this should be a driver decision and not just a single change in
one function. Hence I'm dropping patch 2.

-- 
Kalle Valo

Re: [RESEND, 1/2] wlcore: fix copy-paste bug: assign from src struct not dest

[Kalle Valo]

> Signed-off-by: Giel van Schijndel <me@mortis.eu>
> Reported-at: http://www.viva64.com/en/b/0299/

Thanks, applied to wireless-drivers-next.git.

Kalle Valo