Move the ARRAY_AND_SIZE() macro from arch/arm/mach-pxa/generic.h
to a more useful position in include/linux/kernel.h. This macro
is very useful to registration functions that take an array and
the number of array elements in it as consecutive arguments.

The macro also should ensure that mistakes where the wrong array
is used to the ARRAY_SIZE() macro is passed. It also makes it
easier to avoid wrapping registration function arguments.

Signed-off-by: Ben Dooks <ben-linux@fluff.org>

Index: linux-2.6.27-rc6-quilt4/arch/arm/mach-pxa/generic.h
===================================================================
--- linux-2.6.27-rc6-quilt4.orig/arch/arm/mach-pxa/generic.h	2008-09-18 14:16:47.000000000 +0100
+++ linux-2.6.27-rc6-quilt4/arch/arm/mach-pxa/generic.h	2008-09-18 14:16:52.000000000 +0100
@@ -29,8 +29,6 @@ extern int pxa_last_gpio;
 	mi->bank[__nr].size = (__size), \
 	mi->bank[__nr].node = (((unsigned)(__start) - PHYS_OFFSET) >> 27)
 
-#define ARRAY_AND_SIZE(x)	(x), ARRAY_SIZE(x)
-
 #ifdef CONFIG_PXA25x
 extern unsigned pxa25x_get_clk_frequency_khz(int);
 extern unsigned pxa25x_get_memclk_frequency_10khz(void);
Index: linux-2.6.27-rc6-quilt4/include/linux/kernel.h
===================================================================
--- linux-2.6.27-rc6-quilt4.orig/include/linux/kernel.h	2008-09-18 14:16:06.000000000 +0100
+++ linux-2.6.27-rc6-quilt4/include/linux/kernel.h	2008-09-18 14:16:31.000000000 +0100
@@ -43,6 +43,7 @@ extern const char linux_proc_banner[];
 #define IS_ALIGNED(x, a)		(((x) & ((typeof(x))(a) - 1)) == 0)
 
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
+#define ARRAY_AND_SIZE(arr) (arr), ARRAY_SIZE(arr)
 
 #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
 #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'
--

[ message continues ]

Just like ARRAY_SIZE, it is misnamed.

And it isn't obvious to what it expands. Hopefully arm people will
remove it. :-)
--

[ message continues ]

This is handy to use, saving several key strokes and making the line
shorter. If it's not obvious to what it expands, there must be some
fix for it?
--

[ message continues ]

[Eric Miao - Fri, Sep 19, 2008 at 02:38:21AM +0800]
| On Thu, Sep 18, 2008 at 11:06 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
| > On Thu, Sep 18, 2008 at 02:24:47PM +0100, Ben Dooks wrote:
| >> Move the ARRAY_AND_SIZE() macro from arch/arm/mach-pxa/generic.h
| >> to a more useful position in include/linux/kernel.h. This macro
| >> is very useful to registration functions that take an array and
| >> the number of array elements in it as consecutive arguments.
| >>
| >> The macro also should ensure that mistakes where the wrong array
| >> is used to the ARRAY_SIZE() macro is passed. It also makes it
| >> easier to avoid wrapping registration function arguments.
| >
| >> --- linux-2.6.27-rc6-quilt4.orig/include/linux/kernel.h
| >> +++ linux-2.6.27-rc6-quilt4/include/linux/kernel.h
| >> @@ -43,6 +43,7 @@ extern const char linux_proc_banner[];
| >>  #define IS_ALIGNED(x, a)             (((x) & ((typeof(x))(a) - 1)) == 0)
| >>
| >>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
| >> +#define ARRAY_AND_SIZE(arr) (arr), ARRAY_SIZE(arr)
| >
| > Just like ARRAY_SIZE, it is misnamed.
| >
| 
| Any hint about the correct spelling?
| 
| > And it isn't obvious to what it expands. Hopefully arm people will
| > remove it. :-)
| 
| This is handy to use, saving several key strokes and making the line
| shorter. If it's not obvious to what it expands, there must be some
| fix for it?
| 

well, it seems it's not that good to use ARRAY_AND_SIZE at all.
Yes it's short but quite frankly - hiding number of args is not
that good.

example

	static void ssp_send_cmd(uint32_t *cmd, int num);

called as

	ssp_send_cmd(ARRAY_AND_SIZE(lcd_panel_on));

thanks it's not that spreaded across kernel.
Someday it could lead to ARRAY_AND_SIZE_CHECK_IF_EXIST_AND_PANIC :)

I hope you agree with me.

		- Cyrill -
--

[ message continues ]

Probably that not gonna happen.

without ARRAY_AND_SIZE:

        ssp_send_cmd(lcd_panel_on, ARRAY_SIZE(lcd_panel_on));

with:

        ssp_send_cmd(ARRAY_AND_SIZE(lcd_panel_on));

where you don't have to repeat the array name. I have to admit
that a macro expanding to something like an argument list instead
of a single variable or something  is not a good idea. But, we are
using C, and there's no easy way just to pass the array itself,
otherwise one may come up with:

        ssp_send_cmd(lcd_panel_on);

ssp_send_cmd(array a)
{
        int size = a.length();

........
}

I'm not trying to buy anyone anything, just illustrate this, and see
if anyone else is interested in doing so.
--

[ message continues ]

[Eric Miao - Fri, Sep 19, 2008 at 03:22:13PM +0800]
| On Fri, Sep 19, 2008 at 2:54 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > [Eric Miao - Fri, Sep 19, 2008 at 02:38:21AM +0800]
| > | On Thu, Sep 18, 2008 at 11:06 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
| > | > On Thu, Sep 18, 2008 at 02:24:47PM +0100, Ben Dooks wrote:
| > | >> Move the ARRAY_AND_SIZE() macro from arch/arm/mach-pxa/generic.h
| > | >> to a more useful position in include/linux/kernel.h. This macro
| > | >> is very useful to registration functions that take an array and
| > | >> the number of array elements in it as consecutive arguments.
| > | >>
| > | >> The macro also should ensure that mistakes where the wrong array
| > | >> is used to the ARRAY_SIZE() macro is passed. It also makes it
| > | >> easier to avoid wrapping registration function arguments.
| > | >
| > | >> --- linux-2.6.27-rc6-quilt4.orig/include/linux/kernel.h
| > | >> +++ linux-2.6.27-rc6-quilt4/include/linux/kernel.h
| > | >> @@ -43,6 +43,7 @@ extern const char linux_proc_banner[];
| > | >>  #define IS_ALIGNED(x, a)             (((x) & ((typeof(x))(a) - 1)) == 0)
| > | >>
| > | >>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
| > | >> +#define ARRAY_AND_SIZE(arr) (arr), ARRAY_SIZE(arr)
| > | >
| > | > Just like ARRAY_SIZE, it is misnamed.
| > | >
| > |
| > | Any hint about the correct spelling?
| > |
| > | > And it isn't obvious to what it expands. Hopefully arm people will
| > | > remove it. :-)
| > |
| > | This is handy to use, saving several key strokes and making the line
| > | shorter. If it's not obvious to what it expands, there must be some
| > | fix for it?
| > |
| >
| > well, it seems it's not that good to use ARRAY_AND_SIZE at all.
| > Yes it's short but quite frankly - hiding number of args is not
| > that good.
| >
| > example
| >
| >        static void ssp_send_cmd(uint32_t *cmd, int num);
| >
| > called as
| >
| >        ...
[ message continues ]

...

Exactly, we generally program in C --- not in cpp.
-- 
Stefan Richter
-=====-==--- =--= =--==
http://arcgraph.de/sr/
--

[ message continues ]

My vote is for ARRAY_AND_SIZE to spread far and wide across the land.
ARRAY_SIZE is already very safe, as it has a __must_be_array macro
built in. So ARRAY_AND_SIZE is even safer, as it prevents you from
mixing up two different arrays. It also reduces line length and makes
driver and device (usually platform_device) registration code easier
to read.
--

[ message continues ]

It also spreads ARRAY_SIZE misnaming futher.

It introduces one more core macro and quite pointless one. I can't
personally recall a single bug where sizeof() was taken from another
array.

It creates interesting confusion point: ARRAY_AND_SIZE is about array
and it's size. What ARRAY_SIZE is about then?

You want default argument values in fact, but C is C.

	Alexey "(array (nr (length array)))" Dobriyan
--

[ message continues ]

You still haven't explained what's misnamed about it, nor suggested a 

You haven't written a lot of machine definitions then.  When adding 
platform devices for an embedded platform one has to write a lot of 
boilerplate like this:

     platform_add_devices(n30_devices, ARRAY_SIZE(n30_devices));

and it is much too easy to copy paste that line and miss one of the 

ARRAY_AND_SIZE -> (An) array and (its) size

ARRAY_SIZE -> (The) array size

Sure, you could write ARRAY_AND_ITS_SIZE, but would that really make 
anyone happy?  Cobol went out of fashion a long time ago.

   /Christer

--

[ message continues ]

Christer, _I_ was complaining not about naming
but about hiding function arguments. I suppose
it's better to define some inline wrapper for
platform_add_devices then use such a macro.

If you google a bit you may find that I was asking
someday why don't we define alias for memset when
we just it as _clearing_ routine ie memset(x, 0, sizeof(x)).

		- Cyrill -
--

[ message continues ]

Sorry about that.  I should have commented the earlier one.

In my opinion, making platform_add_devices into a magic macro is 
actually worse, since the same construct (array, ARRAY_SIZE(array)) is 
used in many places, so one would have to do the same thing over and 
over again for every function.  In that case it's better to have to 
learn one macro once, and the ALL_CAPITALS should make it obvious that 
it is a macro.

  /Christer

--

[ message continues ]

[Christer Weinigel - Sat, Sep 20, 2008 at 04:28:19PM +0200]

Well, can't agree with you :) It's my _presonal_ opinion.
You could define it as

static inline int platform_add_devices_array(struct platform_device **devs)
{
	return platform_add_devices(devs, ARRAY_SIZE(devs));
}

for me it would look much better then hide args by MACRO.
And I don't feel any hard about to use platform_add_devices
with TWO arguments. But I repeat - it's my _personal_ opinion.

		- Cyrill -
--

[ message continues ]

Won't work.  You would have to use a macro.  The above would turn into:

      platform_add_devices(devs, 1);

or would if the __must_be_array check didn't catch it.

   /Christer
--

[ message continues ]

Ah...indeed, my bad :) gcc is not that smart (yet). So there only
the macro is possible (just forget that it will be different
namespace scope). Anyway even having it like a macro would be
better then hiding args.

		- Cyrill -
--

[ message continues ]

[Cyrill Gorcunov - Sat, Sep 20, 2008 at 09:12:16PM +0400]
| [Christer Weinigel - Sat, Sep 20, 2008 at 06:38:51PM +0200]
| > Cyrill Gorcunov wrote:
| >> [Christer Weinigel - Sat, Sep 20, 2008 at 04:28:19PM +0200]
| >> ...
| >>> In my opinion, making platform_add_devices into a magic macro is   
| >>> actually worse, since the same construct (array, ARRAY_SIZE(array)) 
| >>> is  used in many places, so one would have to do the same thing over 
| >>> and  over again for every function.  In that case it's better to have 
| >>> to  learn one macro once, and the ALL_CAPITALS should make it obvious 
| >>> that  it is a macro.
| >
| >> Well, can't agree with you :) It's my _presonal_ opinion.
| >> You could define it as
| >>
| >> static inline int platform_add_devices_array(struct platform_device **devs)
| >> {
| >> 	return platform_add_devices(devs, ARRAY_SIZE(devs));
| >> }
| >
| > Won't work.  You would have to use a macro.  The above would turn into:
| >
| >      platform_add_devices(devs, 1);
| >
| > or would if the __must_be_array check didn't catch it.
| >
| >   /Christer
| >
| 
| Ah...indeed, my bad :) gcc is not that smart (yet). So there only
| the macro is possible (just forget that it will be different
| namespace scope). Anyway even having it like a macro would be
| better then hiding args.
| 
| 		- Cyrill -

ie I mean

#define platform_add_devices_array(devs) \
	platform_add_devices(devs, ARRAY_SIZE(devs))

which should satisfy the requirements. Please note that I'm not
trying to say it should be done like that - just a propose which
looks for me much better the ARRAY_AND_SIZE in context of routine
argument.

ps: i was to use macro before inline func but someone inside me
was talking - hey, use function, not macro... and I gave up :)

		- Cyrill -
--

[ message continues ]

ARRAY_LENGTH and ARRAY_AND_LENGTH would be better names IMHO.

AIUI the usual convention is to use :-
- "size" for the size in the sizeof sense; i.e. (in most 
implementations) the size in _bytes_,
- "length" for the size in the sense of the number of _elements_ in an 
array (sizeof(array) / sizeof((array)[0])) which seems to be the 
intention here.

Cheers,
Chris


--

[ message continues ]