2015-01-04 19:10:29

by Giel van Schijndel

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

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



2015-01-07 22:16:40

by Johannes Berg

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

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


2015-01-11 10:22:50

by Eliad Peller

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

On Fri, Jan 9, 2015 at 7:03 PM, Kalle Valo <[email protected]> wrote:
> Giel van Schijndel <[email protected]> writes:
>
>> This highlights the differences (e.g. the bug fixed in the previous
>> commit).
>>
>> Signed-off-by: Giel van Schijndel <[email protected]>
>> ---
>> 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.

2015-01-09 17:03:51

by Kalle Valo

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

Giel van Schijndel <[email protected]> writes:

> This highlights the differences (e.g. the bug fixed in the previous
> commit).
>
> Signed-off-by: Giel van Schijndel <[email protected]>
> ---
> 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

2015-01-04 19:10:29

by Giel van Schijndel

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

---
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


2015-01-07 19:39:02

by Giel van Schijndel

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

Signed-off-by: Giel van Schijndel <[email protected]>
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


2015-01-23 17:07:18

by Kalle Valo

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


> Signed-off-by: Giel van Schijndel <[email protected]>
> Reported-at: http://www.viva64.com/en/b/0299/

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

Kalle Valo

2015-01-05 09:18:01

by Kalle Valo

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

Giel van Schijndel <[email protected]> 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 <[email protected]>

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

--
Kalle Valo

2015-01-04 23:08:00

by Giel van Schijndel

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

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 <[email protected]>

--
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


Attachments:
(No filename) (362.00 B)
signature.asc (181.00 B)
Digital signature
Download all attachments

2015-01-05 09:17:09

by Kalle Valo

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

Giel van Schijndel <[email protected]> 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

2015-01-07 19:18:39

by Giel van Schijndel

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

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 <[email protected]> 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


Attachments:
(No filename) (1.86 kB)
signature.asc (181.00 B)
Digital signature
Download all attachments

2015-01-07 23:07:06

by Arend van Spriel

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

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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/


2015-01-07 18:40:58

by Andy Shevchenko

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

On Sun, Jan 4, 2015 at 8:00 PM, Giel van Schijndel <[email protected]> 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 [email protected]
> 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

2015-01-15 13:23:50

by Kalle Valo

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

Eliad Peller <[email protected]> writes:

> On Fri, Jan 9, 2015 at 7:03 PM, Kalle Valo <[email protected]> wrote:
>> Giel van Schijndel <[email protected]> writes:
>>
>>> This highlights the differences (e.g. the bug fixed in the previous
>>> commit).
>>>
>>> Signed-off-by: Giel van Schijndel <[email protected]>
>>> ---
>>> 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

2015-01-11 20:41:20

by Giel van Schijndel

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

On Sun, Jan 11, 2015 at 12:22:49 +0200, Eliad Peller wrote:
> On Fri, Jan 9, 2015 at 7:03 PM, Kalle Valo <[email protected]> wrote:
>> Giel van Schijndel <[email protected]> writes:
>>> This highlights the differences (e.g. the bug fixed in the previous
>>> commit).
>>>
>>> Signed-off-by: Giel van Schijndel <[email protected]>
>>> ---
>>> 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


Attachments:
(No filename) (3.70 kB)
signature.asc (181.00 B)
Digital signature
Download all attachments

2015-01-04 23:13:03

by Giel van Schijndel

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

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

Forgot to:
Signed-off-by: Giel van Schijndel <[email protected]>

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


Attachments:
(No filename) (200.00 B)
signature.asc (181.00 B)
Digital signature
Download all attachments

2015-01-05 09:54:42

by Johannes Berg

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

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


2015-01-07 19:39:08

by Giel van Schijndel

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

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

Signed-off-by: Giel van Schijndel <[email protected]>
---
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