* Re: [KJ] [third][PATCH 3/4][2.6 drivers/net/skfp/smt.c:]replace SIZE
2007-07-16 14:06 [KJ] [third][PATCH 3/4][2.6 drivers/net/skfp/smt.c:]replace SIZE acripps
@ 2007-07-16 17:06 ` walter harms
2007-07-16 17:24 ` Nishanth Aravamudan
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: walter harms @ 2007-07-16 17:06 UTC (permalink / raw)
To: kernel-janitors
acripps wrote:
> Removed the N_SMT_PLEN macro in favor of the more generic ARRAY_SIZE(arr)
> defined in kernel.h
> Compile tested, no issues found.
> Signed-off-by: Aaron Cripps <cripps@cs.mun.ca>
>
> diff --git a/drivers/net/skfp/smt.c b/drivers/net/skfp/smt.c
> index 75afc1f..a1c49ed 100644
> --- a/drivers/net/skfp/smt.c
> +++ b/drivers/net/skfp/smt.c
> @@ -1654,8 +1654,6 @@ static const struct smt_pdef {
> { SMT_P4053, 0, SWAP_SMT_P4053 } ,
> } ;
>
> -#define N_SMT_PLEN (sizeof(smt_pdef)/sizeof(smt_pdef[0]))
> -
> int smt_check_para(struct s_smc *smc, struct smt_header *sm,
> const u_short list[])
> {
> @@ -1881,7 +1879,7 @@ void smt_swap_para(struct smt_header *sm, int len, int direction)
> if (plen < 0)
> break ;
> plen += PARA_LEN ;
> - for (i = N_SMT_PLEN, pd = smt_pdef; i ; i--,pd++) {
> + for (i = ARRAY_SIZE(smt_pdef), pd = smt_pdef; i ; i--,pd++) {
> if (pd->ptype = type)
> break ;
> }
your patch seems ok but the stile here ...
pd=smt_pdef;
for(i=ARRAY_SIZE(smt_pdef);i;i--) {
if (pd->ptype = type)
break ;
else pd++;
}
normally i would say it should be more like for(i=0;i<ARRAY_SIZE(smt_pdef);i++) (rule of least surprise)
but i have no clue why he did it not this way.
re,
walter
_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
"subscribe kernel-janitors" in message body and follow instructions.
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [KJ] [third][PATCH 3/4][2.6 drivers/net/skfp/smt.c:]replace SIZE
2007-07-16 14:06 [KJ] [third][PATCH 3/4][2.6 drivers/net/skfp/smt.c:]replace SIZE acripps
2007-07-16 17:06 ` walter harms
@ 2007-07-16 17:24 ` Nishanth Aravamudan
2007-07-16 17:25 ` Matthew Wilcox
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Nishanth Aravamudan @ 2007-07-16 17:24 UTC (permalink / raw)
To: kernel-janitors
On 16.07.2007 [19:06:36 +0200], walter harms wrote:
>
>
> acripps wrote:
> > Removed the N_SMT_PLEN macro in favor of the more generic ARRAY_SIZE(arr)
> > defined in kernel.h
> > Compile tested, no issues found.
> > Signed-off-by: Aaron Cripps <cripps@cs.mun.ca>
> >
> > diff --git a/drivers/net/skfp/smt.c b/drivers/net/skfp/smt.c
> > index 75afc1f..a1c49ed 100644
> > --- a/drivers/net/skfp/smt.c
> > +++ b/drivers/net/skfp/smt.c
> > @@ -1654,8 +1654,6 @@ static const struct smt_pdef {
> > { SMT_P4053, 0, SWAP_SMT_P4053 } ,
> > } ;
> >
> > -#define N_SMT_PLEN (sizeof(smt_pdef)/sizeof(smt_pdef[0]))
> > -
> > int smt_check_para(struct s_smc *smc, struct smt_header *sm,
> > const u_short list[])
> > {
> > @@ -1881,7 +1879,7 @@ void smt_swap_para(struct smt_header *sm, int len, int direction)
> > if (plen < 0)
> > break ;
> > plen += PARA_LEN ;
> > - for (i = N_SMT_PLEN, pd = smt_pdef; i ; i--,pd++) {
> > + for (i = ARRAY_SIZE(smt_pdef), pd = smt_pdef; i ; i--,pd++) {
> > if (pd->ptype = type)
> > break ;
> > }
>
> your patch seems ok but the stile here ...
>
> pd=smt_pdef;
> for(i=ARRAY_SIZE(smt_pdef);i;i--) {
> if (pd->ptype = type)
> break ;
> else pd++;
> }
>
> normally i would say it should be more like
> for(i=0;i<ARRAY_SIZE(smt_pdef);i++) (rule of least surprise) but i
> have no clue why he did it not this way.
Well, I think immediately following this loop is
if (i && pd->pswap) {
smt_string_swap(p+PARA_LEN,pd->pswap,len) ;
}
Which would have been different if the loop went the other way.
Certainly possible either way, though.
Thanks,
Nish
--
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center
_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
"subscribe kernel-janitors" in message body and follow instructions.
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [KJ] [third][PATCH 3/4][2.6 drivers/net/skfp/smt.c:]replace SIZE
2007-07-16 14:06 [KJ] [third][PATCH 3/4][2.6 drivers/net/skfp/smt.c:]replace SIZE acripps
2007-07-16 17:06 ` walter harms
2007-07-16 17:24 ` Nishanth Aravamudan
@ 2007-07-16 17:25 ` Matthew Wilcox
2007-07-16 17:33 ` Cripps
2007-07-16 17:48 ` Matthew Wilcox
4 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2007-07-16 17:25 UTC (permalink / raw)
To: kernel-janitors
On Mon, Jul 16, 2007 at 07:06:36PM +0200, walter harms wrote:
> your patch seems ok but the stile here ...
>
> pd=smt_pdef;
> for(i=ARRAY_SIZE(smt_pdef);i;i--) {
> if (pd->ptype = type)
> break ;
> else pd++;
> }
>
> normally i would say it should be more like for(i=0;i<ARRAY_SIZE(smt_pdef);i++) (rule of least surprise)
> but i have no clue why he did it not this way.
I can think of three possible reasons:
1. Speed. Many processors have a 'branch if not zero' instruction.
You can often eliminate one instruction per loop by counting down to
zero instead of up to N.
2. Duplicate entries. If you want to find the last item in the list
which has ptype = type, you need to start at the end, not the front.
3. Speed. If you're likely to be removing entries from a list in the
opposite order to which you added them (a stack rather than a queue),
you'll find it more quickly on average if you start at the end than at
the beginning.
There may be other reasons for iterating in the opposite direction;
those are just three possible reasons that popped into my head when you
asked the question.
--
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
"subscribe kernel-janitors" in message body and follow instructions.
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [KJ] [third][PATCH 3/4][2.6 drivers/net/skfp/smt.c:]replace SIZE
2007-07-16 14:06 [KJ] [third][PATCH 3/4][2.6 drivers/net/skfp/smt.c:]replace SIZE acripps
` (2 preceding siblings ...)
2007-07-16 17:25 ` Matthew Wilcox
@ 2007-07-16 17:33 ` Cripps
2007-07-16 17:48 ` Matthew Wilcox
4 siblings, 0 replies; 6+ messages in thread
From: Cripps @ 2007-07-16 17:33 UTC (permalink / raw)
To: kernel-janitors
I just assumed that the original maintainer of the code had a good reason
for writing the loop in the way he did. and cleaned up the piece that
pertained to my project. The way it is now still compiles with no errors, so
it must not be *entirely* broken (if at all).
Thanks for the comments. If the piece of code in question can actually use
some cleaning up, maybe you should get in contact with the maintainer and
talk it out ... as I've learned from previous attempts though, that change
is beyond the scope of this patch, and as such shouldn't be touched in this
patch. :)
-acripps
On 7/16/07, Matthew Wilcox <matthew@wil.cx> wrote:
>
> On Mon, Jul 16, 2007 at 07:06:36PM +0200, walter harms wrote:
> > your patch seems ok but the stile here ...
> >
> > pd=smt_pdef;
> > for(i=ARRAY_SIZE(smt_pdef);i;i--) {
> > if (pd->ptype = type)
> > break ;
> > else pd++;
> > }
> >
> > normally i would say it should be more like
> for(i=0;i<ARRAY_SIZE(smt_pdef);i++) (rule of least surprise)
> > but i have no clue why he did it not this way.
>
> I can think of three possible reasons:
>
> 1. Speed. Many processors have a 'branch if not zero' instruction.
> You can often eliminate one instruction per loop by counting down to
> zero instead of up to N.
>
> 2. Duplicate entries. If you want to find the last item in the list
> which has ptype = type, you need to start at the end, not the front.
>
> 3. Speed. If you're likely to be removing entries from a list in the
> opposite order to which you added them (a stack rather than a queue),
> you'll find it more quickly on average if you start at the end than at
> the beginning.
>
> There may be other reasons for iterating in the opposite direction;
> those are just three possible reasons that popped into my head when you
> asked the question.
>
> --
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours. We can't possibly take such
> a retrograde step."
>
_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
"subscribe kernel-janitors" in message body and follow instructions.
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [KJ] [third][PATCH 3/4][2.6 drivers/net/skfp/smt.c:]replace SIZE
2007-07-16 14:06 [KJ] [third][PATCH 3/4][2.6 drivers/net/skfp/smt.c:]replace SIZE acripps
` (3 preceding siblings ...)
2007-07-16 17:33 ` Cripps
@ 2007-07-16 17:48 ` Matthew Wilcox
4 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2007-07-16 17:48 UTC (permalink / raw)
To: kernel-janitors
On Mon, Jul 16, 2007 at 05:33:10PM +0000, Cripps wrote:
> I just assumed that the original maintainer of the code had a good reason
> for writing the loop in the way he did. and cleaned up the piece that
> pertained to my project. The way it is now still compiles with no errors, so
> it must not be *entirely* broken (if at all).
> Thanks for the comments. If the piece of code in question can actually use
> some cleaning up, maybe you should get in contact with the maintainer and
> talk it out ... as I've learned from previous attempts though, that change
> is beyond the scope of this patch, and as such shouldn't be touched in this
> patch. :)
Absolutely right.
--
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
"subscribe kernel-janitors" in message body and follow instructions.
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 6+ messages in thread