Re: lots of code could be simplified by using ARRAY_SIZE()

["Robert P. J. Day" <rpjday@mindspring.com>]

(i'm not *trying* to belabour this issue ... i am merely succeeding)

On Sat, 16 Dec 2006, Tim Schmielau wrote:

> On Sat, 16 Dec 2006, Robert P. J. Day wrote:
...
> > ... it's amazing the variation that you find beyond the obvious:
> >
> > $ grep -Er "sizeof.*/.*sizeof" . | less
> >
> > ...
> > ./net/key/af_key.c:     sa->sadb_sa_len = sizeof(struct sadb_sa)/sizeof(uint64_t);
> > ./net/xfrm/xfrm_policy.c:       int len = sizeof(struct xfrm_selector) / sizeof(u32);
> > ./net/core/flow.c:      const int n_elem = sizeof(struct flowi) / sizeof(flow_compare_t);
> > ./net/ipv4/netfilter/arp_tables.c:      for (i = 0; i < sizeof(*arp)/sizeof(__u32); i++)
> > ./net/ipv4/af_inet.c:#define INETSW_ARRAY_LEN (sizeof(inetsw_array) / sizeof(struct inet_protosw))
> > ./drivers/net/wireless/ray_cs.c:        .num_standard   = sizeof(ray_handler)/sizeof(iw_handler),
> >
>
> Of the above, af_inet.c and ray_cs.c seem to be good candidates for
> ARRAY_SIZE. You might even remove the INETSW_ARRAY_LEN #define in
> af_inet.c altogether, since ARRAY_SIZE(inetsw_array) is quite
> readable.

note that the above examples i listed were just a *few* of the
examples that didn't match the most common variants:

  sizeof(fubar) / sizeof(fubar[0])
  sizeof(fubar) / sizeof(*fubar)

i just did that to show that, even if i can run a script to handle the
most common variants, there would be lots of manual cleanup left.

> From a first glance, af_key.c is ok but might profit from a comment
> in include/linux/pfkeyv2.h saying that sadb_msg_len is measured in
> 64-bit words per RFC 2367. Though documenting the structs in
> pfkeyv2.h would be quite a bit different from what you initially
> intended...

in fact, i just emailed a short CodingStyle note to randy dunlap
(since he seemed to be heavily into the coding style stuff),
suggesting that a short note be added strongly recommending that one
should use ARRAY_SIZE wherever possible and, if not possible, a
comment should be added explaining why not, if it seems to be useful.

> So, if you have some time to spend on this, manual inspection would
> probably be the most useful thing, since any automatic sed tricks
> will only replace what a human ready would easily understand as
> well.

true enough, but if the most common variants can be handled
automatically, then the remainder would stand out more obviously and
could be manually handled from there.

> If you manually generate cleanup patches, it would be very good to
> check that compilation with allyesconfig generates identical code
> before and after before feeding them through the respective
> maintainers.

i'm actually in the process of trying that as we speak, at least with
the automatic cleanup.  there's no way i'm going to try to get into
manual cleanup with all of those weird variants.  life's too short for
that.  :-)

rday
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/