[RFC] Add __arraycount from NetBSD to sys/cdefs.h

Tue Sep 9 21:03:35 UTC 2014

On Tue, Sep 09, 2014 at 03:13:45AM -0700, yaneurabeya at gmail.com wrote:
> Hi kib@!
> 
> On Sep 9, 2014, at 1:33, Konstantin Belousov <kostikbel at gmail.com> wrote:
> 
> > On Mon, Sep 08, 2014 at 09:29:54AM -0700, yaneurabeya at gmail.com wrote:
> >> On Sep 3, 2014, at 21:17, Warner Losh <imp at bsdimp.com> wrote:
> >> 
> >>> On Sep 3, 2014, at 9:45 PM, Garrett Cooper <yaneurabeya at gmail.com> wrote:
> >>> 
> >>>> Hi all,
> >>>>  In order to ease porting code and reduce divergence with NetBSD
> >>>> when importing code (a large chunk of which for me are tests), I would
> >>>> like to move nitems to sys/cdefs.h and alias __arraycount to nitems.
> >>>>  Here's the __arraycount #define in lib/libnetbsd/sys/cdefs.h:
> >>>> 
> >>>> 44 /*
> >>>> 45  * Return the number of elements in a statically-allocated array,
> >>>> 46  * __x.
> >>>> 47  */
> >>>> 48 #define __arraycount(__x)       (sizeof(__x) / sizeof(__x[0]))
> >>>> 
> >>>>  Here's the nitems #define in sys/sys/param.h:
> >>>> 
> >>>> 277 #define nitems(x)       (sizeof((x)) / sizeof((x)[0]))
> >>>> 
> >>>>  sys/cdefs.h gets pulled in automatically with sys/param.h, so
> >>>> anything using nitems will continue to function like before (see below
> >>>> for more details). I've attached a patch which addresses all hardcoded
> >>>> definitions in the tree added by FreeBSD developers.
> >>>>  If there aren't any major concerns with my proposed change, I'll
> >>>> put it up for review on Phabricator.
> >>>> Thank you!
> >>>> -Garrett
> >>>> 
> >>>> $ cat cdefs_pound_define.c
> >>>> #include <sys/param.h>
> >>>> 
> >>>> #ifdef _SYS_CDEFS_H_
> >>>> #warning "sys/cdefs.h has been included"
> >>>> #endif
> >>>> $ cc -c cdefs_pound_define.c
> >>>> cdefs_pound_define.c:4:2: warning: "sys/cdefs.h has been included" [-W#warnings]
> >>>> #warning "sys/cdefs.h has been included"
> >>>> ^
> >>>> 1 warning generated.
> >>>> $ cc -D_KERNEL -c cdefs_pound_define.c
> >>>> cdefs_pound_define.c:4:2: warning: "sys/cdefs.h has been included" [-W#warnings]
> >>>> #warning "sys/cdefs.h has been included"
> >>>> ^
> >>>> 1 warning generated.
> >>>> $ gcc -c cdefs_pound_define.c
> >>>> cdefs_pound_define.c:4:2: warning: #warning "sys/cdefs.h has been included"
> >>>> $ gcc -D_KERNEL -c cdefs_pound_define.c
> >>>> cdefs_pound_define.c:4:2: warning: #warning "sys/cdefs.h has been included?
> >>> 
> >>> I wouldn?t bother changing the nitems #define. There?s no need, really, to do that.
> >> 
> >> Rethinking my proposal, I agree. I had lofty hopes for unifying the macros, but the functional duplication (1 line) is harmless.
> >> 
> >>> I?d also be more inclined to believe the test if you tested what the thing does rather than test for an artificial, implementation defined side effect.
> >> 
> >> Sure. I provided a lazy proof instead of a full proof :).
> >> 
> >>> But honestly the amount of duplication saved here is rather tiny?
> >> 
> >> Indeed! Thank you for the input :) ? I?ve attached a new patch which doesn?t disturb nitems or sys/param.h.
> > 
> > Since this is seriously discussed on arch@, I think it is worth look
> > at the following:
> > 
> > git grep -e 'sizeof.*/sizeof' -- | grep -v '^contrib' | grep '#.*define' | wc -l
> >     118
> > These are only defines, not uses, not counting direct use of
> > sizeof(x)/sizeof(x[0]) in the code.
> 
> It?s a bit less painful if you omit cddl, crypto, and gnu:
> 
> $ git grep -e 'sizeof.*/sizeof' -- | egrep -v '^(cddl|contrib|crypto|gnu)' | grep '#.*define' | wc -l
>      105
> 
> > Not all of them are for equiv macros, but significant part repeats ntimes()
> > in variations.  At least Linuxish ARRAY_SIZE/DRM_ARRAY_SIZE is much more
> > popular than __arraysize() which appears three (!) times.
> 
> Hmm.. that?s a bit surprising.
> 
> > I do not see any reason to pollute central header cdefs.h with sparcely
> > used macro for some compat code.
> 
> This is one compelling reason ? it?s used 225 times in the test code in NetBSD, spanning 92 files, and 15 times spanning 10 files in the rump kernel.
> 
> $ grep -rl __arraycount netbsd/src/tests/ | wc -l; grep -r __arraycount netbsd/src/tests/ | wc -l
>       92
>      225
> $ grep -rl __arraycount netbsd/src/sys/rump | wc -l; grep -r __arraycount netbsd/src/sys/rump | wc -l
>       10
>       15
> 
> My goal is to pull in as much test code from NetBSD that I?ve done on my github fork and apply it to FreeBSD with as little deviations as possible, wherever possible. The less we deviate and can push back and forth between FreeBSD and NetBSD, the easier it will be in the long run for both parties to develop and maintain test code and other code from NetBSD.
> 
> That being said, there might be a compromise that could be struck where the compat code can stay confined to libnetbsd. If I change the code bsd.test.mk or contributed code to explicitly use -include or -I, then point to the lib/libnetbsd/sys/cdefs.h header, then this could become a non-issue:
> 
>        -include filename
>            Adds an implicit #include into the predefines buffer which is read
>            before the source file is preprocessed.
> 
>        -Idirectory
>            Add the specified directory to the search path for include files.
> 
> Does that seem like an appropriate solution?
If it is isolated to tests, I have no objections nor comments.

> 
> Thanks!
> -Garrett
> 
> PS For what it?s worth, nitems use has caught on like wildfire (it beats out __arraycount and the ad hoc sizeof):
> 
> $ git grep -e nitems -- | egrep -v '^(cddl|contrib|crypto|gnu)' | wc -l
>      551

It might be worth to clean up the __arraycount or other equivalent
macros in the non-contributed code (only because the issue is already
discussed much more than it worth).  E.g., sys/kern/stack_protector.c
can happily use nitems(), I am sure that there are much more such
places.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20140910/1fd52c1b/attachment.sig>