Date: Tue, 9 Sep 2014 03:13:45 -0700 From: yaneurabeya@gmail.com To: Konstantin Belousov <kostikbel@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: <AB0899A0-02B1-4952-8F8E-D4AB4AADED21@gmail.com> In-Reply-To: <20140909083333.GA2737@kib.kiev.ua> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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?
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
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - https://gpgtools.org
iQEcBAEBCgAGBQJUDtLZAAoJEMZr5QU6S73e4xUH/jjB6W8ng18g5kMRZ2K/6ySi
WPvmKZE3FTpAXzKSFtT5OJo9T1rqfV4+YhsncXkvUhtlLCDuyDjJwqTuWKRdhg47
zDRHSjVOEMH7InWN+OzaI8FfDC1TYI++hKfMHcdUYs8zgLxOWYTrKV/ZPgBFRDE0
PDWA4E+c4956Jaqd2Ixyrb9vY1S0gaUW2Gq1yrxNf9+66UXyWCIfZ0qRUEDPcU65
MtUMwJ6cHcyJZ9Yso144GSZXAZQOn/PNe1/+3AroJj/9DzYQGj39XlgNLBHdVzPs
dU2y3oMFkvgKNGM5c2REqGZJbznE9JBVutUthx60Vn0qjATqsnvh09elo6l+tTs=
=nxsF
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AB0899A0-02B1-4952-8F8E-D4AB4AADED21>