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",
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",
>
>
>
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
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
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
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.
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
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
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