trivial: use ARRAY_SIZE
in [Kernel]
|
Prev: security:key.c Fix warning: variable 'key' set but not used
Next: block:cryptoloop Fix warning: variable 'cipher' set but not used
From: walter harms on 28 Jun 2010 09:30 walter harms schrieb: > > Kulikov Vasiliy schrieb: >> Change sizeof(x) / sizeof(*x) to ARRAY_SIZE(x). >> >> Signed-off-by: Kulikov Vasiliy <segooon(a)gmail.com> >> --- >> drivers/staging/otus/hal/hpreg.c | 17 +++++++---------- >> 1 files changed, 7 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/staging/otus/hal/hpreg.c b/drivers/staging/otus/hal/hpreg.c >> index da3b774..9b04653 100644 >> --- a/drivers/staging/otus/hal/hpreg.c >> +++ b/drivers/staging/otus/hal/hpreg.c >> @@ -29,9 +29,6 @@ >> #include "hpreg.h" >> #include "hpusb.h" >> >> -/* used throughout this file... */ >> -#define N(a) (sizeof(a) / sizeof(a[0])) >> - >> #define HAL_MODE_11A_TURBO HAL_MODE_108A >> #define HAL_MODE_11G_TURBO HAL_MODE_108G >> >> @@ -1557,7 +1554,7 @@ u8_t GetWmRD(u16_t regionCode, u16_t channelFlag, REG_DOMAIN *rd) >> u64_t flags = NO_REQ; >> REG_DMN_PAIR_MAPPING *regPair = NULL; >> >> - for (i = 0, found = 0; (i < N(regDomainPairs)) && (!found); i++) { >> + for (i = 0, found = 0; (i < ARRAY_SIZE(regDomainPairs)) && (!found); i++) { >> if (regDomainPairs[i].regDmnEnum == regionCode) { >> regPair = ®DomainPairs[i]; >> found = 1; > > > > This looks odd, i do not see the rest of the code but perhaps a break would help > to eleminate the "found" ?? > > just my two cents, > wh > 1554 u8_t GetWmRD(u16_t regionCode, u16_t channelFlag, REG_DOMAIN *rd) 1555 { 1556 s16_t i, found, regDmn; 1557 u64_t flags=NO_REQ; 1558 REG_DMN_PAIR_MAPPING *regPair=NULL; 1559 1560 for (i=0, found=0; (i<N(regDomainPairs))&&(!found); i++) 1561 { 1562 if (regDomainPairs[i].regDmnEnum == regionCode) 1563 { 1564 regPair = ®DomainPairs[i]; 1565 found = 1; 1566 } 1567 } 1568 if (!found) 1569 { 1570 zm_debug_msg1("Failed to find reg domain pair ", regionCode); 1571 return FALSE; 1572 } This is would stop at the first hit and work without found. Otherwise someone could add an {NULL} at the end of the array und use a while () that would eliminate the need for ARRAY_SIZE also. re, wh for (i=0; i<ARRAY_SIZE(regDomainPairs);i++ ) if (regDomainPairs[i].regDmnEnum == regionCode) { regPair = ®DomainPairs[i]; break ; } if (!regPair) { zm_debug_msg1("Failed to find reg domain pair ", regionCode); return FALSE; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Miller, Mike (OS Dev) on 28 Jun 2010 15:10 > -----Original Message----- > From: Kulikov Vasiliy [mailto:segooon(a)gmail.com] > Sent: Monday, June 28, 2010 6:55 AM > To: trivial(a)kernel.org > Cc: Kernel Janitors; Miller, Mike (OS Dev); Jens Axboe; > Stephen M. Cameron; Andrew Morton; Patterson, Andrew D > (LeftHand Networks); ISS StorageDev; linux-kernel(a)vger.kernel.org > Subject: [PATCH 01/16] trivial: use ARRAY_SIZE > > Change sizeof(x) / sizeof(*x) to ARRAY_SIZE(x). > > Signed-off-by: Kulikov Vasiliy <segooon(a)gmail.com> Acked-by: Mike Miller <mike.miller(a)hp.com> > --- > drivers/block/cciss.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c > index 51ceaee..e1e7143 100644 > --- a/drivers/block/cciss.c > +++ b/drivers/block/cciss.c > @@ -335,7 +335,7 @@ static void > cciss_map_sg_chain_block(ctlr_info_t *h, CommandList_struct > *c, static const char *raid_label[] = { "0", "4", "1(1+0)", > "5", "5+1", "ADG", > "UNKNOWN" > }; > -#define RAID_UNKNOWN (sizeof(raid_label) / sizeof(raid_label[0])-1) > +#define RAID_UNKNOWN (ARRAY_SIZE(raid_label)-1) > > #ifdef CONFIG_PROC_FS > > -- > 1.7.0.4 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Henk de Groot on 28 Jun 2010 17:00 Op 28-6-2010 15:24, Dan Carpenter schreef: > On Mon, Jun 28, 2010 at 05:15:11PM +0400, Kulikov Vasiliy wrote: > >> On Mon, Jun 28, 2010 at 14:52 +0200, Dan Carpenter wrote: >> >>> On Mon, Jun 28, 2010 at 03:55:41PM +0400, Kulikov Vasiliy wrote: >>> >>>> Change sizeof(x) / sizeof(*x) to ARRAY_SIZE(x). >>>> >>>> Signed-off-by: Kulikov Vasiliy<segooon(a)gmail.com> >>>> --- >>>> drivers/staging/wlags49_h2/hcf.c | 2 +- >>>> 1 files changed, 1 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/drivers/staging/wlags49_h2/hcf.c b/drivers/staging/wlags49_h2/hcf.c >>>> index 390628c..c4fe0ec 100644 >>>> --- a/drivers/staging/wlags49_h2/hcf.c >>>> +++ b/drivers/staging/wlags49_h2/hcf.c >>>> @@ -502,7 +502,7 @@ HCF_STATIC hcf_16* BASED xxxx[ ] = { >>>> #endif // MSF_COMPONENT_ID >>>> NULL //endsentinel >>>> }; >>>> -#define xxxx_PRI_IDENTITY_OFFSET (sizeof(xxxx)/sizeof(xxxx[0]) - 3) >>>> +#define xxxx_PRI_IDENTITY_OFFSET (ARRAY_SIZE(xxxx) - 3) >>>> >>>> >>> I would say the more critical problem with this macro is that it doesn't >>> work unless you name all your arrays "xxxx[]" so it encourages sub par >>> variable names. >>> >>> You could do: >>> #define PRI_IDENTITY_OFFSET(x) (ARRAY_SIZE(x) - 3) >>> >> Look at the patch: >> >> >>>> @@ -502,7 +502,7 @@ HCF_STATIC hcf_16* BASED xxxx[ ] = { >>>> >> There is an array called 'xxxx' and macro xxxx_PRI_IDENTITY_OFFSET is defined >> after array definition. This magic macroconstant is used in the code to get >> elements of xxxx. >> > Right right. But xxxx is a stupid name for a variable. I wanted to > poke my eyes out with a fork. > > Not your fault obviously. Your patch doesn't make it worse so I'm fine > with it as far as it goes... > > regards, > dan carpenter > The whole HCF library code is full of this funny stuff. My guess is the code is generated and macro's are used to access arrays in a uniform way. At least that's the only thing I can think of why it is as it is. Only the original developers at Agere can tell the real story. The same with structure definitions. You could send this to the IOCCC easily (it is only too big to qualify)... I think the best way to fix this is to take all this funny stuff out and make it simplified readable code. Currently its very hard to see through all the macro layers what actually happens which make is unmaintainable. Changing details is okay, but will never make any of this more readable. When porting the driver from the original Agere 2.4 kernel code to the 2.6 driver today I only need to change one or two casts, the rest of the HCF code is untouched. The only Linux code is in the wl_* files which coding style looks quite different from the rest and is actually readable. Kind regards, Henk. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Simon Horman on 28 Jun 2010 21:00 On Mon, Jun 28, 2010 at 03:22:49PM +0200, walter harms wrote: > > > walter harms schrieb: > > > > Kulikov Vasiliy schrieb: > >> Change sizeof(x) / sizeof(*x) to ARRAY_SIZE(x). > >> > >> Signed-off-by: Kulikov Vasiliy <segooon(a)gmail.com> > >> --- > >> drivers/staging/otus/hal/hpreg.c | 17 +++++++---------- > >> 1 files changed, 7 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/staging/otus/hal/hpreg.c b/drivers/staging/otus/hal/hpreg.c > >> index da3b774..9b04653 100644 > >> --- a/drivers/staging/otus/hal/hpreg.c > >> +++ b/drivers/staging/otus/hal/hpreg.c > >> @@ -29,9 +29,6 @@ > >> #include "hpreg.h" > >> #include "hpusb.h" > >> > >> -/* used throughout this file... */ > >> -#define N(a) (sizeof(a) / sizeof(a[0])) > >> - > >> #define HAL_MODE_11A_TURBO HAL_MODE_108A > >> #define HAL_MODE_11G_TURBO HAL_MODE_108G > >> > >> @@ -1557,7 +1554,7 @@ u8_t GetWmRD(u16_t regionCode, u16_t channelFlag, REG_DOMAIN *rd) > >> u64_t flags = NO_REQ; > >> REG_DMN_PAIR_MAPPING *regPair = NULL; > >> > >> - for (i = 0, found = 0; (i < N(regDomainPairs)) && (!found); i++) { > >> + for (i = 0, found = 0; (i < ARRAY_SIZE(regDomainPairs)) && (!found); i++) { > >> if (regDomainPairs[i].regDmnEnum == regionCode) { > >> regPair = ®DomainPairs[i]; > >> found = 1; > > > > > > > > This looks odd, i do not see the rest of the code but perhaps a break would help > > to eleminate the "found" ?? > > > > just my two cents, > > wh > > > 1554 u8_t GetWmRD(u16_t regionCode, u16_t channelFlag, REG_DOMAIN *rd) > 1555 { > 1556 s16_t i, found, regDmn; > 1557 u64_t flags=NO_REQ; > 1558 REG_DMN_PAIR_MAPPING *regPair=NULL; > 1559 > 1560 for (i=0, found=0; (i<N(regDomainPairs))&&(!found); i++) > 1561 { > 1562 if (regDomainPairs[i].regDmnEnum == regionCode) > 1563 { > 1564 regPair = ®DomainPairs[i]; > 1565 found = 1; > 1566 } > 1567 } > 1568 if (!found) > 1569 { > 1570 zm_debug_msg1("Failed to find reg domain pair ", regionCode); > 1571 return FALSE; > 1572 } > > > This is would stop at the first hit and work without found. > Otherwise someone could add an {NULL} at the end of the array > und use a while () that would eliminate the need for ARRAY_SIZE also. Either of those suggestions sounds fine to me, but it sounds like it should be in a separate patch. > > re, > wh > > for (i=0; i<ARRAY_SIZE(regDomainPairs);i++ ) > if (regDomainPairs[i].regDmnEnum == regionCode) > { regPair = ®DomainPairs[i]; break ; } > > if (!regPair) { > zm_debug_msg1("Failed to find reg domain pair ", regionCode); > return FALSE; > } > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Simon Horman on 28 Jun 2010 21:00
On Mon, Jun 28, 2010 at 03:55:20PM +0400, Kulikov Vasiliy wrote: > Change sizeof(x) / sizeof(*x) to ARRAY_SIZE(x). > > Signed-off-by: Kulikov Vasiliy <segooon(a)gmail.com> > --- > drivers/staging/otus/hal/hpmain.c | 28 ++++++++++------------------ > 1 files changed, 10 insertions(+), 18 deletions(-) > > diff --git a/drivers/staging/otus/hal/hpmain.c b/drivers/staging/otus/hal/hpmain.c > index 5f412e0..6d2d358 100644 > --- a/drivers/staging/otus/hal/hpmain.c > +++ b/drivers/staging/otus/hal/hpmain.c > @@ -430,7 +430,7 @@ void zfInitPhy(zdev_t* dev, u32_t frequency, u8_t bw40) > * Register setting by mode > */ > > - entries = sizeof(ar5416Modes) / sizeof(*ar5416Modes); > + entries = ARRAY_SIZE(ar5416Modes); > zm_msg1_scan(ZM_LV_2, "Modes register setting entries=", entries); > for (i=0; i<entries; i++) > { > @@ -496,7 +496,7 @@ void zfInitPhy(zdev_t* dev, u32_t frequency, u8_t bw40) > /* > * Common Register setting > */ > - entries = sizeof(ar5416Common) / sizeof(*ar5416Common); > + entries = ARRAY_SIZE(ar5416Common); > for (i=0; i<entries; i++) > { > reg_write(ar5416Common[i][0], ar5416Common[i][1]); > @@ -506,7 +506,7 @@ void zfInitPhy(zdev_t* dev, u32_t frequency, u8_t bw40) > /* > * RF Gain setting by freqIndex > */ > - entries = sizeof(ar5416BB_RfGain) / sizeof(*ar5416BB_RfGain); > + entries = ARRAY_SIZE(ar5416BB_RfGain); > for (i=0; i<entries; i++) > { > reg_write(ar5416BB_RfGain[i][0], ar5416BB_RfGain[i][freqIndex]); > @@ -963,7 +963,6 @@ u32_t reverse_bits(u32_t chan_sel) > /* Bank 0 1 2 3 5 6 7 */ > void zfSetRfRegs(zdev_t* dev, u32_t frequency) > { > - u16_t entries; > u16_t freqIndex = 0; > u16_t i; > > @@ -984,33 +983,28 @@ void zfSetRfRegs(zdev_t* dev, u32_t frequency) > } > > #if 1 > - entries = sizeof(otusBank) / sizeof(*otusBank); > - for (i=0; i<entries; i++) > + for (i=0; i<ARRAY_SIZE(otusBank); i++) As you are changing this line, I wonder if you might also fix the white-space at the same time. In particular, a space on either side of '=' and '<'. + for (i = 0; i < ARRAY_SIZE(otusBank); i++) Ditto multiple times below. > { > reg_write(otusBank[i][0], otusBank[i][freqIndex]); > } > #else > /* Bank0 */ > - entries = sizeof(ar5416Bank0) / sizeof(*ar5416Bank0); > - for (i=0; i<entries; i++) > + for (i=0; i<ARRAY_SIZE(ar5416Bank0); i++) > { > reg_write(ar5416Bank0[i][0], ar5416Bank0[i][1]); > } > /* Bank1 */ > - entries = sizeof(ar5416Bank1) / sizeof(*ar5416Bank1); > - for (i=0; i<entries; i++) > + for (i=0; i<ARRAY_SIZE(ar5416Bank1); i++) > { > reg_write(ar5416Bank1[i][0], ar5416Bank1[i][1]); > } > /* Bank2 */ > - entries = sizeof(ar5416Bank2) / sizeof(*ar5416Bank2); > - for (i=0; i<entries; i++) > + for (i=0; i<ARRAY_SIZE(ar5416Bank2); i++) > { > reg_write(ar5416Bank2[i][0], ar5416Bank2[i][1]); > } > /* Bank3 */ > - entries = sizeof(ar5416Bank3) / sizeof(*ar5416Bank3); > - for (i=0; i<entries; i++) > + for (i=0; i<ARRAY_SIZE(ar5416Bank3); i++) > { > reg_write(ar5416Bank3[i][0], ar5416Bank3[i][freqIndex]); > } > @@ -1018,14 +1012,12 @@ void zfSetRfRegs(zdev_t* dev, u32_t frequency) > reg_write (0x98b0, 0x00000013); > reg_write (0x98e4, 0x00000002); > /* Bank6 */ > - entries = sizeof(ar5416Bank6) / sizeof(*ar5416Bank6); > - for (i=0; i<entries; i++) > + for (i=0; i<ARRAY_SIZE(ar5416Bank6); i++) > { > reg_write(ar5416Bank6[i][0], ar5416Bank6[i][freqIndex]); > } > /* Bank7 */ > - entries = sizeof(ar5416Bank7) / sizeof(*ar5416Bank7); > - for (i=0; i<entries; i++) > + for (i=0; i<ARRAY_SIZE(ar5416Bank7); i++) > { > reg_write(ar5416Bank7[i][0], ar5416Bank7[i][1]); > } > -- > 1.7.0.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |