Date: Thu, 2 Jun 2016 18:41:54 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Ed Schouten <ed@nuxi.nl>, Ed Schouten <ed@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r301071 - head/sys/sys Message-ID: <20160602174338.Q1850@besplex.bde.org> In-Reply-To: <20160601162238.GM38613@kib.kiev.ua> References: <201605311905.u4VJ5geL053766@repo.freebsd.org> <20160601183101.X1028@besplex.bde.org> <CABh_MKkXH-F4LepiVnmMptHOCbC=OH8FDvia8Zv-0mHUYrnbtA@mail.gmail.com> <20160601162238.GM38613@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 1 Jun 2016, Konstantin Belousov wrote: > On Wed, Jun 01, 2016 at 05:44:47PM +0200, Ed Schouten wrote: >> ... >>>> - In our implementation, struct sigevent::sigev_notify_attributes has >>>> type "void *" instead of "pthread_attr_t *". My guess is that this was >>>> done to prevent pulling in the pthread types, but this can easily be >>>> avoided by using the underlying structure types. >>> >>> Not easily, since the tags of the underlying struct types are in the >>> application namespace, at least up to POSIX 2001. >> >> Yeah, it's quite unfortunate that we use structure types starting with >> 'pthread'. They should have had leading underscores. But in my opinion >> that's not a problem specific to this change; it's a problem with our >> pthread implementation in general. >> >>>> +#include <sys/_pthreadtypes.h> Indeed. The problem became larger in r146824 when these types started to be declared unconditionally in <sys/types.h>. Another bug in sys/_pthreadtypes.h is that it says that the prefixes pthread_ and PTHREAD_ are reserved for use in header symbols. Actually, they are only reserved for use in <pthread.h>. This shouldn't be documented in the general header. The non-broken parts of general header depend on symbols ending with _t being reserved, not on this. >>> This gives the following pollution (which breaks almost everything since >>> <sys/types.h> includes this header: >>> - struct tag names pthread* >>> - struct member names state and mutex >> >> Yes. It would have made so much more sense if a header like >> <sys/_types.h> would have defined all pthread types as __pthread_t, >> __pthread_mutex_t, etc. That way there would have been a way to expose >> just pthread_t and pthread_attr_t without pulling in the rest. > No, it wouldn't. Putting everything in sys/_types.h (or machine/_types.h) is convenient, and it avoids proliferation of headers, but it gives a different bloat problem: every header ends up declaring every type, but with an spelling in the implementation namespace. Then the number of types almost doubles when you declare only almost all types again in the application namespace. > Replace the typedefs with the forward-struct names by the void *. > The only other change would be the libthr, where some casts might > be needed. > > Use void * directly in signal.h if possible. Pointers to incomplete structures give more type safety than void *. Except for the problem with struct tags, just using them is best. Unlike for typedefs, we don't need a 5-line ifdef for every use to avoid redeclaratiion errors. sys/signal.h seems to use a pthread type just once. This type can be declared as 'struct pthread_attr *'. Actually, signal.h already used the correct method, except this was obfuscated using a private typedef (__pthread_t), and at some point when POSIX apparently started explicitly requiring <signal.h> to declare pthread_t, <signal.h> was polluted to match. We use ifdefs to a fault to minimise the scope of typedefs like size_t. Not doing this for the pthread types is inconsistent. For POSIX headers, it is easiest to declare all typedefs *_t in all headers. This may actually be more efficient by avoiding 1 layer of indirect typedefs like __size_t and 2-3 layers of header convolutions and ifdefs. The indirections and convolutuons are still technically needed for non-POSIX headers. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160602174338.Q1850>