RFC: kernel coding style: prefer array to &array[0] ?

RFC: kernel coding style: prefer array to &array[0] ?

[Joe Perches]

It seems most in-kernel uses are 'array' rather than '&array[0]'

Most of the time, using array is simpler to read than &array[0].

Exceptions exists when addresses for consecutive members are
used like func(&array[0], &array[1]);

Should this preference be put into checkpatch and/or CodingStyle?

Here's a possible checkpatch --strict addition
---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 90e1edc..362a9d8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5492,6 +5492,12 @@ sub process {
 			}
 		}
 
+# check for address of array[0] (not '&& array[0]' or &array[0].member)
+		if ($sline =~ /[^\&]&\s*($Ident\s*(?:(?:\-\>|\.)\s*$Ident\s*)*)\s*\[\s*0\s*\]\s*(?!\[|\.|\-\>)/) {
+			CHK("ADDRESSOF_ARRAY",
+			    "Using addressof array '$1' index [0] may be simpler as '$1'\n" . $herecurr);
+		}
+
 # check for semaphores initialized locked
 		if ($line =~ /^.\s*sema_init.+,\W?0\W?\)/) {
 			WARN("CONSIDER_COMPLETION",

Re: RFC: kernel coding style: prefer array to &array[0] ?

[Julia Lawall]

On Tue, 30 Jun 2015, Joe Perches wrote:

> It seems most in-kernel uses are 'array' rather than '&array[0]'
> 
> Most of the time, using array is simpler to read than &array[0].
> 
> Exceptions exists when addresses for consecutive members are
> used like func(&array[0], &array[1]);
> 
> Should this preference be put into checkpatch and/or CodingStyle?

&array[0] looks complicated to me.

julia


> Here's a possible checkpatch --strict addition
> ---
>  scripts/checkpatch.pl | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 90e1edc..362a9d8 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5492,6 +5492,12 @@ sub process {
>  			}
>  		}
>  
> +# check for address of array[0] (not '&& array[0]' or &array[0].member)
> +		if ($sline =~ /[^\&]&\s*($Ident\s*(?:(?:\-\>|\.)\s*$Ident\s*)*)\s*\[\s*0\s*\]\s*(?!\[|\.|\-\>)/) {
> +			CHK("ADDRESSOF_ARRAY",
> +			    "Using addressof array '$1' index [0] may be simpler as '$1'\n" . $herecurr);
> +		}
> +
>  # check for semaphores initialized locked
>  		if ($line =~ /^.\s*sema_init.+,\W?0\W?\)/) {
>  			WARN("CONSIDER_COMPLETION",
> 
> 
> 

Re: RFC: kernel coding style: prefer array to &array[0] ?

[Clemens Ladisch]

Joe Perches wrote:
> It seems most in-kernel uses are 'array' rather than '&array[0]'
>
> Most of the time, using array is simpler to read than &array[0].
>
> Exceptions exists when addresses for consecutive members are
> used like func(&array[0], &array[1]);

I use '&array[0]' when I want to get a pointer to a single object that
happens to be the first one in an array.

> Should this preference be put into checkpatch and/or CodingStyle?

How about the following low-hanging fruit?

  foo(..., &array[0], ARRAY_SIZE(array), ...)


Regards,
Clemens

Re: RFC: kernel coding style: prefer array to &array[0] ?

[Dan Carpenter]

On Wed, Jul 01, 2015 at 01:54:29PM +0200, Clemens Ladisch wrote:
> Joe Perches wrote:
> > It seems most in-kernel uses are 'array' rather than '&array[0]'
> >
> > Most of the time, using array is simpler to read than &array[0].
> >
> > Exceptions exists when addresses for consecutive members are
> > used like func(&array[0], &array[1]);
> 
> I use '&array[0]' when I want to get a pointer to a single object that
> happens to be the first one in an array.

Yeah.  Of course, you're right.  Otherwise it ends up confusing static
checkers if you want the first element or the whole array.

> 
> > Should this preference be put into checkpatch and/or CodingStyle?
> 
> How about the following low-hanging fruit?
> 
>   foo(..., &array[0], ARRAY_SIZE(array), ...)

Yes, to this also.  I doubt checkpatch.pl will find a meaningful number
of these but doing that is annoying thing.

regards,
dan carpenter

Re: RFC: kernel coding style: prefer array to &array[0] ?

[Julia Lawall]

On Wed, 1 Jul 2015, Dan Carpenter wrote:

> On Wed, Jul 01, 2015 at 01:54:29PM +0200, Clemens Ladisch wrote:
> > Joe Perches wrote:
> > > It seems most in-kernel uses are 'array' rather than '&array[0]'
> > >
> > > Most of the time, using array is simpler to read than &array[0].
> > >
> > > Exceptions exists when addresses for consecutive members are
> > > used like func(&array[0], &array[1]);
> >
> > I use '&array[0]' when I want to get a pointer to a single object that
> > happens to be the first one in an array.
>
> Yeah.  Of course, you're right.  Otherwise it ends up confusing static
> checkers if you want the first element or the whole array.
>
> >
> > > Should this preference be put into checkpatch and/or CodingStyle?
> >
> > How about the following low-hanging fruit?
> >
> >   foo(..., &array[0], ARRAY_SIZE(array), ...)
>
> Yes, to this also.  I doubt checkpatch.pl will find a meaningful number
> of these but doing that is annoying thing.

Atcually, I find 236 of them, in 48 files.

julia

Re: RFC: kernel coding style: prefer array to &array[0] ?

[Joe Perches]

On Wed, 2015-07-01 at 14:26 +0200, Julia Lawall wrote:
> On Wed, 1 Jul 2015, Dan Carpenter wrote:
> > On Wed, Jul 01, 2015 at 01:54:29PM +0200, Clemens Ladisch wrote:
> > > Joe Perches wrote:
> > > > It seems most in-kernel uses are 'array' rather than '&array[0]'
> > > >
> > > > Most of the time, using array is simpler to read than &array[0].
> > > >
> > > > Exceptions exists when addresses for consecutive members are
> > > > used like func(&array[0], &array[1]);
> > >
> > > I use '&array[0]' when I want to get a pointer to a single object that
> > > happens to be the first one in an array.
> >
> > Yeah.  Of course, you're right.  Otherwise it ends up confusing static
> > checkers if you want the first element or the whole array.

Right.

> > > > Should this preference be put into checkpatch and/or CodingStyle?

And checkpatch will have no idea what the prototype
for any function is, so this transform is better left
for smarter tools like coccinelle.

The proper answer here is no.

> > > How about the following low-hanging fruit?
> > >
> > >   foo(..., &array[0], ARRAY_SIZE(array), ...)
> >
> > Yes, to this also.  I doubt checkpatch.pl will find a meaningful number
> > of these but doing that is annoying thing.
> 
> Atcually, I find 236 of them, in 48 files.

The uses I found:

drivers/input/touchscreen nas a few

There are some inconsistent uses of 1 vs ARRAY_SIZE
in drivers/mfd/ for "struct mfc_cell" arrays uses for
	mfd_add_devices()

2 in net/netfilter/xt_l2tp.c that could be changed

sound/pci has a couple

sound/soc/codecs has the rest.  These are all the same
form where a macro like SND_SOC_DAPM_MIXER is used.

An example:

#define SND_SOC_DAPM_MIXER(wname, wreg, wshift, winvert, \
	 wcontrols, wncontrols)\
{	.id = snd_soc_dapm_mixer, .name = wname, \
	SND_SOC_DAPM_INIT_REG_VAL(wreg, wshift, winvert), \
	.kcontrol_news = wcontrols, .num_kcontrols = wncontrols}

but is used with wcontrols as either NULL or an array
and ARRAY_SIZE can't be used on NULL.

Perhaps it's appropriate to change the macro (and uses)
removing the last wncontrols argument.  Something like:

#define SND_SOC_DAPM_MIXER(wname, wreg, wshift, winvert, wcontrols)	\
{									\
	.id = snd_soc_dapm_mixer,					\
	.name = wname, 							\
	SND_SOC_DAPM_INIT_REG_VAL(wreg, wshift, winvert),		\
	.kcontrol_news = wcontrols,					\
	.num_kcontrols = (wcontrols) ? ARRAY_SIZE(wcontrols) : 0,	\
}

for example, the uses change from:
	SND_SOC_DAPM_MIXER("HPOut Mix", SND_SOC_NOPM, 0, 0, NULL, 0),
to:
	SND_SOC_DAPM_MIXER("HPOut Mix", SND_SOC_NOPM, 0, 0, NULL),

and from:
	SND_SOC_DAPM_MIXER("Mono Mixer", SND_SOC_NOPM, 0, 0,
			   da7210_dapm_monomix_controls, ARRAY_SIZE(da7210_dapm_monomix_controls)),
to:
	SND_SOC_DAPM_MIXER("Mono Mixer", SND_SOC_NOPM, 0, 0,
			   da7210_dapm_monomix_controls),

or just leave them as-is.

Re: RFC: kernel coding style: prefer array to &array[0] ?

[Dmitry Torokhov]

On Wed, Jul 01, 2015 at 07:53:44AM -0700, Joe Perches wrote:
> On Wed, 2015-07-01 at 14:26 +0200, Julia Lawall wrote:
> > On Wed, 1 Jul 2015, Dan Carpenter wrote:
> > > On Wed, Jul 01, 2015 at 01:54:29PM +0200, Clemens Ladisch wrote:
> > > > Joe Perches wrote:
> > > > > It seems most in-kernel uses are 'array' rather than '&array[0]'
> > > > >
> > > > > Most of the time, using array is simpler to read than &array[0].
> > > > >
> > > > > Exceptions exists when addresses for consecutive members are
> > > > > used like func(&array[0], &array[1]);
> > > >
> > > > I use '&array[0]' when I want to get a pointer to a single object that
> > > > happens to be the first one in an array.
> > >
> > > Yeah.  Of course, you're right.  Otherwise it ends up confusing static
> > > checkers if you want the first element or the whole array.
> 
> Right.
> 
> > > > > Should this preference be put into checkpatch and/or CodingStyle?
> 
> And checkpatch will have no idea what the prototype
> for any function is, so this transform is better left
> for smarter tools like coccinelle.
> 
> The proper answer here is no.
> 
> > > > How about the following low-hanging fruit?
> > > >
> > > >   foo(..., &array[0], ARRAY_SIZE(array), ...)
> > >
> > > Yes, to this also.  I doubt checkpatch.pl will find a meaningful number
> > > of these but doing that is annoying thing.
> > 
> > Atcually, I find 236 of them, in 48 files.
> 
> The uses I found:
> 
> drivers/input/touchscreen nas a few

I got curious so I ran the proposed patch over drivers/input/touchscreen
and it produced the following gems:

CHECK: Using addressof array 'data' index [0] may be simpler as 'data'
#49: FILE: drivers/input/touchscreen/dynapro.c:49:
+#define DYNAPRO_GET_TOUCHED(data) (DYNAPRO_FORMAT_TOUCH_BIT & data[0])


CHECK: Using addressof array 'mtouch->data' index [0] may be simpler as
'mtouch->data'
#97: FILE: drivers/input/touchscreen/mtouch.c:97:
+       if (MTOUCH_FORMAT_TABLET_STATUS_BIT & mtouch->data[0])

... etc.


While below can be written as just "msg" in many cases when you parse
several fields in the structure the original is actually cleaner:

CHECK: Using addressof array 'msg' index [0] may be simpler as 'msg'
#38: FILE: drivers/input/touchscreen/ipaq-micro-ts.c:38:
+                                be16_to_cpup((__be16 *) &msg[0]));

I'd be OK with changing cases like:

CHECK: Using addressof array 'buf' index [0] may be simpler as 'buf'
#232: FILE: drivers/input/touchscreen/zforce_ts.c:232:
+       return zforce_send_wait(ts, &buf[0], ARRAY_SIZE(buf));

Thanks.

-- 
Dmitry

Re: RFC: kernel coding style: prefer array to &array[0] ?

[Joe Perches]

On Wed, 2015-07-01 at 15:33 -0700, Dmitry Torokhov wrote:
> On Wed, Jul 01, 2015 at 07:53:44AM -0700, Joe Perches wrote:
> > On Wed, 2015-07-01 at 14:26 +0200, Julia Lawall wrote:
> > > On Wed, 1 Jul 2015, Dan Carpenter wrote:
> > > > On Wed, Jul 01, 2015 at 01:54:29PM +0200, Clemens Ladisch wrote:
> > > > > Joe Perches wrote:
> > > > > > It seems most in-kernel uses are 'array' rather than '&array[0]'
> > > > > >
> > > > > > Most of the time, using array is simpler to read than &array[0].
> > > > > >
> > > > > > Exceptions exists when addresses for consecutive members are
> > > > > > used like func(&array[0], &array[1]);
> > > > >
> > > > > I use '&array[0]' when I want to get a pointer to a single object that
> > > > > happens to be the first one in an array.
> > > >
> > > > Yeah.  Of course, you're right.  Otherwise it ends up confusing static
> > > > checkers if you want the first element or the whole array.
> > 
> > Right.
> > 
> > > > > > Should this preference be put into checkpatch and/or CodingStyle?
> > 
> > And checkpatch will have no idea what the prototype
> > for any function is, so this transform is better left
> > for smarter tools like coccinelle.
> > 
> > The proper answer here is no.
[]
> CHECK: Using addressof array 'mtouch->data' index [0] may be simpler as
> 'mtouch->data'
> #97: FILE: drivers/input/touchscreen/mtouch.c:97:
> +       if (MTOUCH_FORMAT_TABLET_STATUS_BIT & mtouch->data[0])

The joys of perl parsing.

> CHECK: Using addressof array 'msg' index [0] may be simpler as 'msg'
> #38: FILE: drivers/input/touchscreen/ipaq-micro-ts.c:38:
> +                                be16_to_cpup((__be16 *) &msg[0]));

That's using the first member in an array.

> I'd be OK with changing cases like:
> 
> CHECK: Using addressof array 'buf' index [0] may be simpler as 'buf'
> #232: FILE: drivers/input/touchscreen/zforce_ts.c:232:
> +       return zforce_send_wait(ts, &buf[0], ARRAY_SIZE(buf));

I think cases like those are sensible to change.

cheers, Joe

Re: RFC: kernel coding style: prefer array to &array[0] ?

[Julia Lawall]

Anotherpattern that occurred to me is, eg

info->MS_Status = *(struct MS_STATUS *)&buf[0];

where buf is an array.  I find this in 11 files.

julia