diff mbox

arch/arm/mach-pxa/stargate2.c: use ARRAY_AND_SIZE consistently

Message ID 1376148353-9699-1-git-send-email-Julia.Lawall@lip6.fr (mailing list archive)
State New, archived
Headers show

Commit Message

Julia Lawall Aug. 10, 2013, 3:25 p.m. UTC
From: Julia Lawall <Julia.Lawall@lip6.fr>

Because ARRAY_AND_SIZE changes the apparent arity of a function, if it is
used for one call to a given function, it would be better, if possible, to
use it for all of them.

The semantic patch that fixes this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@call@
identifier f;
expression e;
expression list[n] es;
@@

f(es,ARRAY_AND_SIZE(e),...)

@@
expression e;
identifier call.f;
expression list[call.n] ess;
@@

f(ess,
- e,ARRAY_SIZE(e)
+ ARRAY_AND_SIZE(e)
  ,...)
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 arch/arm/mach-pxa/stargate2.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Comments

Joe Perches Aug. 10, 2013, 3:59 p.m. UTC | #1
On Sat, 2013-08-10 at 17:38 +0200, Julia Lawall wrote:
> On Sat, 10 Aug 2013, Joe Perches wrote:
> > On Sat, 2013-08-10 at 17:25 +0200, Julia Lawall wrote:
> > > From: Julia Lawall <Julia.Lawall@lip6.fr>
> > > Because ARRAY_AND_SIZE changes the apparent arity of a function, if it is
> > > used for one call to a given function, it would be better, if possible, to
> > > use it for all of them.
> > I think it'd be better to remove ARRAY_AND_SIZE instead.
> 
> I can do that, if it is wanted, but there are over 200 uses, and I think 
> it is a little bit positive in that it ensures that the array is passed 
> along with its own size.

Maybe, but if it's generally accepted that it's useful,
and I'm a little dubious because it does hide argument
count in functions where it's used, the 6 #defines should
be centralized in kernel.h

$ git grep "define.*ARRAY_AND_SIZE"
arch/arm/mach-kirkwood/common.h:#define ARRAY_AND_SIZE(x)       (x), ARRAY_SIZE(x)
arch/arm/mach-mmp/common.h:#define ARRAY_AND_SIZE(x)    (x), ARRAY_SIZE(x)
arch/arm/mach-pxa/generic.h:#define ARRAY_AND_SIZE(x)   (x), ARRAY_SIZE(x)
arch/arm/mach-ux500/db8500-regs.h:#define ARRAY_AND_SIZE(x)     (x), ARRAY_SIZE(x)
drivers/pinctrl/pinctrl-lantiq.h:#define ARRAY_AND_SIZE(x)      (x), ARRAY_SIZE(x)
sound/soc/pxa/mioa701_wm9713.c:#define ARRAY_AND_SIZE(x)        (x), ARRAY_SIZE(x)

A small counter-argument against using ARRAY_AND_SIZE:

clkdev_add_table has 60+ uses, only 6 of those with
ARRAY_AND_SIZE.

I think it makes it difficult to do some cocinelle/spatch
transform on clkdev_add_table.
Walter Harms Aug. 10, 2013, 5:49 p.m. UTC | #2
Am 10.08.2013 17:32, schrieb Joe Perches:
> On Sat, 2013-08-10 at 17:25 +0200, Julia Lawall wrote:
>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>>
>> Because ARRAY_AND_SIZE changes the apparent arity of a function, if it is
>> used for one call to a given function, it would be better, if possible, to
>> use it for all of them.
> 
> I think it'd be better to remove ARRAY_AND_SIZE instead.
> 

+1

:)

re,
 wh
Dan Carpenter Aug. 10, 2013, 10:33 p.m. UTC | #3
ARRAY_AND_SIZE() macro is horrible, and I would like it if it were
removed.  What I meant before was just that probably people will
probably complain if we try to remove it.

regards,
dan carpenter
Julia Lawall Aug. 11, 2013, 5:48 a.m. UTC | #4
On Sun, 11 Aug 2013, Dan Carpenter wrote:

> ARRAY_AND_SIZE() macro is horrible, and I would like it if it were
> removed.  What I meant before was just that probably people will
> probably complain if we try to remove it.

Well, I could either wait for someone to defend it, or send a patch 
getting rid of it and see what happens...  I can't see what could go wrong 
with

f(...,
- ARRAY_AND_SIZE(e)
+ e, ARRAY_SIZE(e)
  , ...)

so it should be an easy patch to create.

julia
Walter Harms Aug. 11, 2013, 7:34 a.m. UTC | #5
Am 11.08.2013 07:48, schrieb Julia Lawall:
> On Sun, 11 Aug 2013, Dan Carpenter wrote:
> 
>> ARRAY_AND_SIZE() macro is horrible, and I would like it if it were
>> removed.  What I meant before was just that probably people will
>> probably complain if we try to remove it.
> 
> Well, I could either wait for someone to defend it, or send a patch 
> getting rid of it and see what happens...  I can't see what could go wrong 
> with
> 
> f(...,
> - ARRAY_AND_SIZE(e)
> + e, ARRAY_SIZE(e)
>   , ...)
> 
> so it should be an easy patch to create.
> 


if it is not to much work, send a patch to get rid of it.
It seems all agree that the disadvantages outweighs it
advantages.

just my 2 cents,
 wh
diff mbox

Patch

diff --git a/arch/arm/mach-pxa/stargate2.c b/arch/arm/mach-pxa/stargate2.c
index 62aea3e..ee7a42e 100644
--- a/arch/arm/mach-pxa/stargate2.c
+++ b/arch/arm/mach-pxa/stargate2.c
@@ -608,12 +608,10 @@  static void __init imote2_init(void)
 
 	imote2_stargate2_init();
 
-	platform_add_devices(imote2_devices, ARRAY_SIZE(imote2_devices));
+	platform_add_devices(ARRAY_AND_SIZE(imote2_devices));
 
-	i2c_register_board_info(0, imote2_i2c_board_info,
-				ARRAY_SIZE(imote2_i2c_board_info));
-	i2c_register_board_info(1, imote2_pwr_i2c_board_info,
-				ARRAY_SIZE(imote2_pwr_i2c_board_info));
+	i2c_register_board_info(0, ARRAY_AND_SIZE(imote2_i2c_board_info));
+	i2c_register_board_info(1, ARRAY_AND_SIZE(imote2_pwr_i2c_board_info));
 
 	pxa_set_mci_info(&imote2_mci_platform_data);
 	pxa_set_udc_info(&imote2_udc_info);
@@ -990,8 +988,7 @@  static void __init stargate2_init(void)
 	platform_add_devices(ARRAY_AND_SIZE(stargate2_devices));
 
 	i2c_register_board_info(0, ARRAY_AND_SIZE(stargate2_i2c_board_info));
-	i2c_register_board_info(1, stargate2_pwr_i2c_board_info,
-				ARRAY_SIZE(stargate2_pwr_i2c_board_info));
+	i2c_register_board_info(1, ARRAY_AND_SIZE(stargate2_pwr_i2c_board_info));
 
 	pxa_set_mci_info(&stargate2_mci_platform_data);