Date: Wed, 10 Sep 2014 00:03:29 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: yaneurabeya@gmail.com Cc: "freebsd-arch@FreeBSD.org Arch" <freebsd-arch@freebsd.org> Subject: Re: [RFC] Add __arraycount from NetBSD to sys/cdefs.h Message-ID: <20140909210329.GE2737@kib.kiev.ua> In-Reply-To: <AB0899A0-02B1-4952-8F8E-D4AB4AADED21@gmail.com> References: <CAGHfRMBMPra5YXDn0e83dpVxwnarg3DL8o31xr7DhWv%2BVXskTg@mail.gmail.com> <8D279BDC-7D40-4750-8DA7-A4535DD2E458@bsdimp.com> <146C3E96-7461-4D30-8B7F-5E20F72CF061@gmail.com> <20140909083333.GA2737@kib.kiev.ua> <AB0899A0-02B1-4952-8F8E-D4AB4AADED21@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] On Tue, Sep 09, 2014 at 03:13:45AM -0700, yaneurabeya@gmail.com wrote: > Hi kib@! > > On Sep 9, 2014, at 1:33, Konstantin Belousov <kostikbel@gmail.com> wrote: > > > On Mon, Sep 08, 2014 at 09:29:54AM -0700, yaneurabeya@gmail.com wrote: > >> On Sep 3, 2014, at 21:17, Warner Losh <imp@bsdimp.com> wrote: > >> > >>> On Sep 3, 2014, at 9:45 PM, Garrett Cooper <yaneurabeya@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. [-- Attachment #2 --] -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUD2sgAAoJEJDCuSvBvK1BZaEP/3bYiHQmUOOCUA3O3w0EllCm dJPm7PSfnbd0oAeFm6Y47dtH/napTcbctYGKe+/Dznd2wUz8sgFVhjXynlxBEazl LShyizXEel3c0et5KCZ9bkP58oio9Cd20jhb82KcX8hvvlN97UcuvHr59M9Zpxu6 Vs/p5WCNCv70uTcPn06R0GFO8ldgy2pTPSj9AvavfcNk++rb/rpvkl/Gt2h+74S5 UjiNI7gNn+MACYcgtaDvAbsug2M6X4d0aaS5b0YWuwj7YCx8QG/+1N8R2KCn6SrF r5BmgQ/1ngw7pbazvZ5cdc5AQ+qfMX/EmBeBYwkCthR9+RAh0FNJyRTbXB36D820 1X5nQw9buFZZNrFrAriiSkLQN4lYg4csHgQkErA88pcG8U7yNTnAScqQmFK7iFRK xfm5HeAsUw6qr2naCRoMnz75n0WODP5+TmcT1Np57/4nw5RpYFCexQNP1D0Wd7N4 XK8+OwrtBGgM52aVgJF9Dle9oxJntRtq6bdZyYCq613tS7OmBMd9lavCnWGht6cR BHwMTMs/Y7ijASa5whxHTCTtGQ5FIIVKqNlQT5d1YgzrXiNLARDDu9Z6UYK/UjiZ pbRmyIhdi3o/J5qgLW9pOcI4O6FcDwe0NxRaN+GqjxjSgRUQlGX5D5xAIv0kezuH DkUa3IeS11cnR/wHmKD4 =fmQy -----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140909210329.GE2737>