* [PATCH] ata: Clean up hard coded array size calculation.
@ 2009-11-08 19:30 Thiago Farina
2009-11-08 19:45 ` Thiago Farina
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Thiago Farina @ 2009-11-08 19:30 UTC (permalink / raw)
To: jgarzik; +Cc: mlord, linux-ide, linux-kernel, Thiago Farina
Use ARRAY_SIZE macro of kernel api instead.
Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
drivers/ata/sata_mv.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 6f5093b..a8a7be0 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -2217,7 +2217,7 @@ static unsigned int mv_qc_issue_fis(struct ata_queued_cmd *qc)
int err = 0;
ata_tf_to_fis(&qc->tf, link->pmp, 1, (void *)fis);
- err = mv_send_fis(ap, fis, sizeof(fis) / sizeof(fis[0]));
+ err = mv_send_fis(ap, fis, ARRAY_SIZE(fis));
if (err)
return err;
--
1.6.5.2.150.g1b52a
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-08 19:30 [PATCH] ata: Clean up hard coded array size calculation Thiago Farina
@ 2009-11-08 19:45 ` Thiago Farina
2009-11-10 5:00 ` Mark Lord
2009-11-17 3:17 ` Jeff Garzik
2 siblings, 0 replies; 15+ messages in thread
From: Thiago Farina @ 2009-11-08 19:45 UTC (permalink / raw)
To: jgarzik; +Cc: mlord, linux-ide, linux-kernel, Thiago Farina
On Sun, Nov 8, 2009 at 5:30 PM, Thiago Farina <tfransosi@gmail.com> wrote:
> Use ARRAY_SIZE macro of kernel api instead.
>
> Signed-off-by: Thiago Farina <tfransosi@gmail.com>
Reported-by: Robert P. J. Day <rpjday@crashcourse.ca>
> ---
> drivers/ata/sata_mv.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 6f5093b..a8a7be0 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -2217,7 +2217,7 @@ static unsigned int mv_qc_issue_fis(struct ata_queued_cmd *qc)
> int err = 0;
>
> ata_tf_to_fis(&qc->tf, link->pmp, 1, (void *)fis);
> - err = mv_send_fis(ap, fis, sizeof(fis) / sizeof(fis[0]));
> + err = mv_send_fis(ap, fis, ARRAY_SIZE(fis));
> if (err)
> return err;
>
> --
> 1.6.5.2.150.g1b52a
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-08 19:30 [PATCH] ata: Clean up hard coded array size calculation Thiago Farina
2009-11-08 19:45 ` Thiago Farina
@ 2009-11-10 5:00 ` Mark Lord
2009-11-10 15:51 ` Bartlomiej Zolnierkiewicz
2009-11-11 23:10 ` Jeff Garzik
2009-11-17 3:17 ` Jeff Garzik
2 siblings, 2 replies; 15+ messages in thread
From: Mark Lord @ 2009-11-10 5:00 UTC (permalink / raw)
To: Thiago Farina; +Cc: jgarzik, mlord, linux-ide, linux-kernel
Thiago Farina wrote:
> Use ARRAY_SIZE macro of kernel api instead.
>
> Signed-off-by: Thiago Farina <tfransosi@gmail.com>
> ---
> drivers/ata/sata_mv.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 6f5093b..a8a7be0 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -2217,7 +2217,7 @@ static unsigned int mv_qc_issue_fis(struct ata_queued_cmd *qc)
> int err = 0;
>
> ata_tf_to_fis(&qc->tf, link->pmp, 1, (void *)fis);
> - err = mv_send_fis(ap, fis, sizeof(fis) / sizeof(fis[0]));
> + err = mv_send_fis(ap, fis, ARRAY_SIZE(fis));
> if (err)
> return err;
>
..
What's the point of this ?
There is no "hardcoded array size" there to begin with,
and using that silly macro obscures the actual calculation.
So now, instead of being able to verify correctness at a glance,
I have to go off and research some silly macro and verify that
it does the right thing.
Kind of like all of those "typedef structs" that are abhored around here.
-ml
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-10 5:00 ` Mark Lord
@ 2009-11-10 15:51 ` Bartlomiej Zolnierkiewicz
2009-11-10 16:08 ` Pekka Enberg
2009-11-11 23:10 ` Jeff Garzik
1 sibling, 1 reply; 15+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-11-10 15:51 UTC (permalink / raw)
To: Mark Lord; +Cc: Thiago Farina, jgarzik, mlord, linux-ide, linux-kernel
On Tuesday 10 November 2009 06:00:19 Mark Lord wrote:
> Thiago Farina wrote:
> > Use ARRAY_SIZE macro of kernel api instead.
> >
> > Signed-off-by: Thiago Farina <tfransosi@gmail.com>
> > ---
> > drivers/ata/sata_mv.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> > index 6f5093b..a8a7be0 100644
> > --- a/drivers/ata/sata_mv.c
> > +++ b/drivers/ata/sata_mv.c
> > @@ -2217,7 +2217,7 @@ static unsigned int mv_qc_issue_fis(struct ata_queued_cmd *qc)
> > int err = 0;
> >
> > ata_tf_to_fis(&qc->tf, link->pmp, 1, (void *)fis);
> > - err = mv_send_fis(ap, fis, sizeof(fis) / sizeof(fis[0]));
> > + err = mv_send_fis(ap, fis, ARRAY_SIZE(fis));
> > if (err)
> > return err;
> >
> ..
>
> What's the point of this ?
>
> There is no "hardcoded array size" there to begin with,
> and using that silly macro obscures the actual calculation.
>
> So now, instead of being able to verify correctness at a glance,
I kindly disagree here. ARRAY_SIZE makes code smaller and prevents
subtle bugs in the more complex situations once you learn to always
use it.
[ Using ARRAY_SIZE you no longer have to verify anything and person
looking at the code (which not necessarily is the original author)
immediately knows what was the author's intention. ]
> I have to go off and research some silly macro and verify that
> it does the right thing.
You did it already and the macro name is quite descriptive so you
may as well just ACK the patch now.. ;)
--
Bartlomiej Zolnierkiewicz
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-10 15:51 ` Bartlomiej Zolnierkiewicz
@ 2009-11-10 16:08 ` Pekka Enberg
2009-11-11 22:37 ` Mark Lord
0 siblings, 1 reply; 15+ messages in thread
From: Pekka Enberg @ 2009-11-10 16:08 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Mark Lord, Thiago Farina, jgarzik, mlord, linux-ide, linux-kernel
On Tue, Nov 10, 2009 at 5:51 PM, Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> wrote:
>> I have to go off and research some silly macro and verify that
>> it does the right thing.
>
> You did it already and the macro name is quite descriptive so you
> may as well just ACK the patch now.. ;)
Indeed. We use the "silly macro" everywhere in the kernel for code clarity.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-10 16:08 ` Pekka Enberg
@ 2009-11-11 22:37 ` Mark Lord
2009-11-11 22:50 ` Thiago Farina
0 siblings, 1 reply; 15+ messages in thread
From: Mark Lord @ 2009-11-11 22:37 UTC (permalink / raw)
To: Pekka Enberg
Cc: Bartlomiej Zolnierkiewicz, Thiago Farina, jgarzik, mlord,
linux-ide, linux-kernel
Pekka Enberg wrote:
> On Tue, Nov 10, 2009 at 5:51 PM, Bartlomiej Zolnierkiewicz
> <bzolnier@gmail.com> wrote:
>>> I have to go off and research some silly macro and verify that
>>> it does the right thing.
>> You did it already and the macro name is quite descriptive so you
>> may as well just ACK the patch now.. ;)
>
> Indeed. We use the "silly macro" everywhere in the kernel for code clarity.
..
Looks like unnecessary churn and obfuscation to me.
-ml
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-11 22:37 ` Mark Lord
@ 2009-11-11 22:50 ` Thiago Farina
2009-11-12 23:53 ` Mark Lord
0 siblings, 1 reply; 15+ messages in thread
From: Thiago Farina @ 2009-11-11 22:50 UTC (permalink / raw)
To: Mark Lord
Cc: Pekka Enberg, Bartlomiej Zolnierkiewicz, jgarzik, mlord,
linux-ide, linux-kernel
On Wed, Nov 11, 2009 at 8:37 PM, Mark Lord <liml@rtr.ca> wrote:
> Looks like unnecessary churn and obfuscation to me.
Obfuscation? Are you saying that just because you wrote this code? So
what is the usage of a macro? I think you are having a defensive
position here.
The macro is self-explanatory, ARRAY_SIZE, so the macro gives you the
*size* of the array. You don't need to know how that is done, if you
want, just go to the declaration of the macro.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-10 5:00 ` Mark Lord
2009-11-10 15:51 ` Bartlomiej Zolnierkiewicz
@ 2009-11-11 23:10 ` Jeff Garzik
1 sibling, 0 replies; 15+ messages in thread
From: Jeff Garzik @ 2009-11-11 23:10 UTC (permalink / raw)
To: Mark Lord; +Cc: Thiago Farina, mlord, linux-ide, linux-kernel
On 11/10/2009 12:00 AM, Mark Lord wrote:
> Thiago Farina wrote:
>> Use ARRAY_SIZE macro of kernel api instead.
>>
>> Signed-off-by: Thiago Farina <tfransosi@gmail.com>
>> ---
>> drivers/ata/sata_mv.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
>> index 6f5093b..a8a7be0 100644
>> --- a/drivers/ata/sata_mv.c
>> +++ b/drivers/ata/sata_mv.c
>> @@ -2217,7 +2217,7 @@ static unsigned int mv_qc_issue_fis(struct
>> ata_queued_cmd *qc)
>> int err = 0;
>>
>> ata_tf_to_fis(&qc->tf, link->pmp, 1, (void *)fis);
>> - err = mv_send_fis(ap, fis, sizeof(fis) / sizeof(fis[0]));
>> + err = mv_send_fis(ap, fis, ARRAY_SIZE(fis));
>> if (err)
>> return err;
>>
> ..
>
> What's the point of this ?
>
> There is no "hardcoded array size" there to begin with,
> and using that silly macro obscures the actual calculation.
>
> So now, instead of being able to verify correctness at a glance,
> I have to go off and research some silly macro and verify that
> it does the right thing.
It is a standard cleanup for all kernel code, and it does make the code
more readable to the casual reader / quick skimmer.
Jeff
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-11 22:50 ` Thiago Farina
@ 2009-11-12 23:53 ` Mark Lord
2009-11-13 0:25 ` Jeff Garzik
0 siblings, 1 reply; 15+ messages in thread
From: Mark Lord @ 2009-11-12 23:53 UTC (permalink / raw)
To: Thiago Farina
Cc: Pekka Enberg, Bartlomiej Zolnierkiewicz, jgarzik, mlord,
linux-ide, linux-kernel
Thiago Farina wrote:
> On Wed, Nov 11, 2009 at 8:37 PM, Mark Lord <liml@rtr.ca> wrote:
>> Looks like unnecessary churn and obfuscation to me.
>
> Obfuscation? Are you saying that just because you wrote this code? So
> what is the usage of a macro? I think you are having a defensive
> position here.
..
No, actually I believe Jeff wrote that particular line of code.
Cheers
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-12 23:53 ` Mark Lord
@ 2009-11-13 0:25 ` Jeff Garzik
2009-11-13 0:41 ` Thiago Farina
0 siblings, 1 reply; 15+ messages in thread
From: Jeff Garzik @ 2009-11-13 0:25 UTC (permalink / raw)
To: Mark Lord
Cc: Thiago Farina, Pekka Enberg, Bartlomiej Zolnierkiewicz, mlord,
linux-ide, linux-kernel
On 11/12/2009 06:53 PM, Mark Lord wrote:
> Thiago Farina wrote:
>> On Wed, Nov 11, 2009 at 8:37 PM, Mark Lord <liml@rtr.ca> wrote:
>>> Looks like unnecessary churn and obfuscation to me.
>>
>> Obfuscation? Are you saying that just because you wrote this code? So
>> what is the usage of a macro? I think you are having a defensive
>> position here.
> ..
>
> No, actually I believe Jeff wrote that particular line of code.
You know, Linus actually invented a command for people who enjoy this
game... ;)
git blame drivers/ata/sata_mv.c
shows each line of code, and metadata about each line such as commit id
and author name.
Jeff
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-13 0:25 ` Jeff Garzik
@ 2009-11-13 0:41 ` Thiago Farina
2009-11-14 3:23 ` Mark Lord
0 siblings, 1 reply; 15+ messages in thread
From: Thiago Farina @ 2009-11-13 0:41 UTC (permalink / raw)
To: Jeff Garzik
Cc: Mark Lord, Pekka Enberg, Bartlomiej Zolnierkiewicz, mlord,
linux-ide, linux-kernel
On Thu, Nov 12, 2009 at 10:25 PM, Jeff Garzik <jgarzik@pobox.com> wrote:
> On 11/12/2009 06:53 PM, Mark Lord wrote:
>>
>> Thiago Farina wrote:
>>>
>>> On Wed, Nov 11, 2009 at 8:37 PM, Mark Lord <liml@rtr.ca> wrote:
>>>>
>>>> Looks like unnecessary churn and obfuscation to me.
>>>
>>> Obfuscation? Are you saying that just because you wrote this code? So
>>> what is the usage of a macro? I think you are having a defensive
>>> position here.
>>
>> ..
>>
>> No, actually I believe Jeff wrote that particular line of code.
>
> You know, Linus actually invented a command for people who enjoy this
> game... ;)
>
> git blame drivers/ata/sata_mv.c
>
> shows each line of code, and metadata about each line such as commit id and
> author name.
Sure, and I ran this command before saying that Mark wrote this code.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-13 0:41 ` Thiago Farina
@ 2009-11-14 3:23 ` Mark Lord
0 siblings, 0 replies; 15+ messages in thread
From: Mark Lord @ 2009-11-14 3:23 UTC (permalink / raw)
To: Thiago Farina
Cc: Jeff Garzik, Pekka Enberg, Bartlomiej Zolnierkiewicz, mlord,
linux-ide, linux-kernel
Thiago Farina wrote:
> On Thu, Nov 12, 2009 at 10:25 PM, Jeff Garzik <jgarzik@pobox.com> wrote:
>> On 11/12/2009 06:53 PM, Mark Lord wrote:
>>> Thiago Farina wrote:
>>>> On Wed, Nov 11, 2009 at 8:37 PM, Mark Lord <liml@rtr.ca> wrote:
>>>>> Looks like unnecessary churn and obfuscation to me.
>>>> Obfuscation? Are you saying that just because you wrote this code? So
>>>> what is the usage of a macro? I think you are having a defensive
>>>> position here.
>>> ..
>>>
>>> No, actually I believe Jeff wrote that particular line of code.
>> You know, Linus actually invented a command for people who enjoy this
>> game... ;)
>>
>> git blame drivers/ata/sata_mv.c
>>
>> shows each line of code, and metadata about each line such as commit id and
>> author name.
>
> Sure, and I ran this command before saying that Mark wrote this code.
..
I'm probably just the last chap to change the indentation on it.
It really doesn't look like my style to me. ;)
There were two authors before me there, and that line looks like one of theirs. :)
Cheers
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-08 19:30 [PATCH] ata: Clean up hard coded array size calculation Thiago Farina
2009-11-08 19:45 ` Thiago Farina
2009-11-10 5:00 ` Mark Lord
@ 2009-11-17 3:17 ` Jeff Garzik
2009-11-20 19:15 ` Thiago Farina
2 siblings, 1 reply; 15+ messages in thread
From: Jeff Garzik @ 2009-11-17 3:17 UTC (permalink / raw)
To: Thiago Farina; +Cc: mlord, linux-ide, linux-kernel
On 11/08/2009 02:30 PM, Thiago Farina wrote:
> Use ARRAY_SIZE macro of kernel api instead.
>
> Signed-off-by: Thiago Farina<tfransosi@gmail.com>
> ---
> drivers/ata/sata_mv.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 6f5093b..a8a7be0 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -2217,7 +2217,7 @@ static unsigned int mv_qc_issue_fis(struct ata_queued_cmd *qc)
> int err = 0;
>
> ata_tf_to_fis(&qc->tf, link->pmp, 1, (void *)fis);
> - err = mv_send_fis(ap, fis, sizeof(fis) / sizeof(fis[0]));
> + err = mv_send_fis(ap, fis, ARRAY_SIZE(fis));
> if (err)
applied
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-17 3:17 ` Jeff Garzik
@ 2009-11-20 19:15 ` Thiago Farina
2009-11-20 22:05 ` Jeff Garzik
0 siblings, 1 reply; 15+ messages in thread
From: Thiago Farina @ 2009-11-20 19:15 UTC (permalink / raw)
To: Jeff Garzik; +Cc: mlord, linux-ide, linux-kernel
Hi Jeff,
On Tue, Nov 17, 2009 at 1:17 AM, Jeff Garzik <jgarzik@pobox.com> wrote:
> On 11/08/2009 02:30 PM, Thiago Farina wrote:
>>
>> Use ARRAY_SIZE macro of kernel api instead.
>>
>> Signed-off-by: Thiago Farina<tfransosi@gmail.com>
>> ---
>> drivers/ata/sata_mv.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
>> index 6f5093b..a8a7be0 100644
>> --- a/drivers/ata/sata_mv.c
>> +++ b/drivers/ata/sata_mv.c
>> @@ -2217,7 +2217,7 @@ static unsigned int mv_qc_issue_fis(struct
>> ata_queued_cmd *qc)
>> int err = 0;
>>
>> ata_tf_to_fis(&qc->tf, link->pmp, 1, (void *)fis);
>> - err = mv_send_fis(ap, fis, sizeof(fis) / sizeof(fis[0]));
>> + err = mv_send_fis(ap, fis, ARRAY_SIZE(fis));
>> if (err)
>
> applied
Was it applied to this tree
http://git.kernel.org/?p=linux/kernel/git/jgarzik/libata-dev.git?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ata: Clean up hard coded array size calculation.
2009-11-20 19:15 ` Thiago Farina
@ 2009-11-20 22:05 ` Jeff Garzik
0 siblings, 0 replies; 15+ messages in thread
From: Jeff Garzik @ 2009-11-20 22:05 UTC (permalink / raw)
To: Thiago Farina; +Cc: mlord, linux-ide, linux-kernel
On 11/20/2009 02:15 PM, Thiago Farina wrote:
> Hi Jeff,
>
> On Tue, Nov 17, 2009 at 1:17 AM, Jeff Garzik<jgarzik@pobox.com> wrote:
>> On 11/08/2009 02:30 PM, Thiago Farina wrote:
>>>
>>> Use ARRAY_SIZE macro of kernel api instead.
>>>
>>> Signed-off-by: Thiago Farina<tfransosi@gmail.com>
>>> ---
>>> drivers/ata/sata_mv.c | 2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
>>> index 6f5093b..a8a7be0 100644
>>> --- a/drivers/ata/sata_mv.c
>>> +++ b/drivers/ata/sata_mv.c
>>> @@ -2217,7 +2217,7 @@ static unsigned int mv_qc_issue_fis(struct
>>> ata_queued_cmd *qc)
>>> int err = 0;
>>>
>>> ata_tf_to_fis(&qc->tf, link->pmp, 1, (void *)fis);
>>> - err = mv_send_fis(ap, fis, sizeof(fis) / sizeof(fis[0]));
>>> + err = mv_send_fis(ap, fis, ARRAY_SIZE(fis));
>>> if (err)
>>
>> applied
>
> Was it applied to this tree
> http://git.kernel.org/?p=linux/kernel/git/jgarzik/libata-dev.git?
Yes.
Jeff
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2009-11-20 22:05 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-11-08 19:30 [PATCH] ata: Clean up hard coded array size calculation Thiago Farina
2009-11-08 19:45 ` Thiago Farina
2009-11-10 5:00 ` Mark Lord
2009-11-10 15:51 ` Bartlomiej Zolnierkiewicz
2009-11-10 16:08 ` Pekka Enberg
2009-11-11 22:37 ` Mark Lord
2009-11-11 22:50 ` Thiago Farina
2009-11-12 23:53 ` Mark Lord
2009-11-13 0:25 ` Jeff Garzik
2009-11-13 0:41 ` Thiago Farina
2009-11-14 3:23 ` Mark Lord
2009-11-11 23:10 ` Jeff Garzik
2009-11-17 3:17 ` Jeff Garzik
2009-11-20 19:15 ` Thiago Farina
2009-11-20 22:05 ` Jeff Garzik
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.