[PATCH] ata: Clean up hard coded array size calculation.

[Thiago Farina]

Use ARRAY_SIZE macro of kernel api instead.

Signed-off-by: Thiago Farina <tfra...@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

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

[Thiago Farina]

On Sun, Nov 8, 2009 at 5:30 PM, Thiago Farina <tfra...@gmail.com> wrote:
> Use ARRAY_SIZE macro of kernel api instead.
>
> Signed-off-by: Thiago Farina <tfra...@gmail.com>

Reported-by: Robert P. J. Day <rpj...@crashcourse.ca>

[Mark Lord]

Thiago Farina wrote:
> Use ARRAY_SIZE macro of kernel api instead.
>
> Signed-off-by: Thiago Farina <tfra...@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

[Bartlomiej Zolnierkiewicz]

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 <tfra...@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

[Pekka Enberg]

On Tue, Nov 10, 2009 at 5:51 PM, Bartlomiej Zolnierkiewicz
<bzol...@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.

[Mark Lord]

Pekka Enberg wrote:
> On Tue, Nov 10, 2009 at 5:51 PM, Bartlomiej Zolnierkiewicz
> <bzol...@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

[Thiago Farina]

On Wed, Nov 11, 2009 at 8:37 PM, Mark Lord <li...@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.

[Jeff Garzik]

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 <tfra...@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

[Mark Lord]

Thiago Farina wrote:
> On Wed, Nov 11, 2009 at 8:37 PM, Mark Lord <li...@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

[Jeff Garzik]

On 11/12/2009 06:53 PM, Mark Lord wrote:
> Thiago Farina wrote:
>> On Wed, Nov 11, 2009 at 8:37 PM, Mark Lord <li...@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

[Thiago Farina]

On Thu, Nov 12, 2009 at 10:25 PM, Jeff Garzik <jga...@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 <li...@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.

[Mark Lord]

Thiago Farina wrote:
> On Thu, Nov 12, 2009 at 10:25 PM, Jeff Garzik <jga...@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 <li...@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

[Jeff Garzik]

On 11/08/2009 02:30 PM, Thiago Farina wrote:
> Use ARRAY_SIZE macro of kernel api instead.
>
> Signed-off-by: Thiago Farina<tfra...@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

[Thiago Farina]

Hi Jeff,

On Tue, Nov 17, 2009 at 1:17 AM, Jeff Garzik <jga...@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<tfra...@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?

[Jeff Garzik]

On 11/20/2009 02:15 PM, Thiago Farina wrote:
> Hi Jeff,
>
> On Tue, Nov 17, 2009 at 1:17 AM, Jeff Garzik<jga...@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<tfra...@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