Date: Wed, 19 Sep 2018 04:37:10 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Brooks Davis <brooks@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r338751 - head/include Message-ID: <20180919025337.F2618@besplex.bde.org> In-Reply-To: <201809181531.w8IFVOSW089586@repo.freebsd.org> References: <201809181531.w8IFVOSW089586@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 18 Sep 2018, Brooks Davis wrote: > Log: > Fix C11 and POSIX 1003.1b-1993 compliance in time.h > > Only expose timespec_get in C11, C++17, or BSD code. Always define > struct timespect if defining timespec_get. > > PR: 231425 > Reviewed by: kib > Approved by: re (gjb) > Differential Revision: https://reviews.freebsd.org/D17174 This still has a high density of namespace pollution and style bugs, not all in old code: > Modified: head/include/time.h > ============================================================================== > --- head/include/time.h Tue Sep 18 15:01:21 2018 (r338750) > +++ head/include/time.h Tue Sep 18 15:31:24 2018 (r338751) > @@ -207,9 +207,13 @@ time_t posix2time(time_t t); > #include <xlocale/_time.h> > #endif > > +#if defined(__BSD_VISIBLE) || __ISO_C_VISIBLE >= 2011 || \ > + (defined(cplusplus) && cplusplus >= 201703) > +#include <sys/_timespec.h> Lexical and organizational style bugs: The includes are scattered over the file and here they are not separated by blank lines. Long before here, the larger header <sys/timespec.h> has already been included in the POSIX.1b section. The formatting in that section is better than here, but the header itself is much worse. It gives the following undocumented namespace pollution in <time.h>: TIMEVAL_TO_TIMESPEC() and TIMESPEC_TO_TIMEVAL(). There are both under __BSD_VISIBLE, so the pollution is not very bad. However, the reason for existence of the 3 little headers <sys/_timespec.h>, <sys/timespec.h> and <sys/_timeval.h> is to avoid namespace pollution. 3 is too many even if they worked. Adding pollution to _timespec.h since FreeBSD-5 turned the split of the timespec headers into nonsense. Originally, _timespec.h declared struct timespec but not time_t, and timespec.h existed to also declare time_t. timespec.h always had the polluting conversion macros. Now _timespec.h is further from matching its name (it declares timespec, not _timespec), and timespec.h is unrelated to its name (it also declares timespec, but is not needed for that, leaving its only use as defining the conversion macros). Also, much the same commit that added the declaration of time_t to _timespec.h also increased the pollution in <sys/stat.h>. timespec in stat.h was new in POSIX.1-2008. <sys/stat.h> has always had massive namespace pollution in FreeBSD, by including the massively polluted <sys/time.h>. struct timespec was a small part of this pollution before 2008 and became non-pollution in 2008. Old versions of stat.h were at least careful not to use struct timespec in stat structs except under __BSD_VISIBLE. In my version, the pollution is mostly fixed by removing the incude of <sys/time.h> and including only <sys/_timespec.h> in the !BSD_VISIBLE case (stat.h doesn't use it, but some buggy applications do?) and <sys/timespec.h> in the BSD_VISIBLE case (buggy applications certainly need this). > /* ISO/IEC 9899:201x 7.27.2.5 The timespec_get function */ Style bug: the comment does less than echo the code. The code gives the correct condition, and the comment only echos 1/3 of the condition but with excessive details (this is not the place to give IEC numbers for everything, and even the definition of the feature test macros in <sys/cdefs.h> only has 1 IEC number. I think /dev/null is the best place to give these numbers). The code is short, so it is obvious that this only applies to the timespec_get() function in it. > #define TIME_UTC 1 /* time elapsed since epoch */ Style bug: space instead of tab after #define. <time.h> has this bug in 16 out of 26 #defines (all of the newer ones). > int timespec_get(struct timespec *ts, int base); Style bug: the function name is not indented. <time.h> misformats all its prototypes like this. Namespace pollution: ts and 'base' are in the application namespace. <time.h> doesn't have this bug in most of its prototypes. All of the old ones are correct. timer_oshandle_np() has this bug. Newer prototypes are often unsorted within their subsections. > +#endif > > __END_DECLS The C standard has a general problem with specifying reserved names for struct members. I couldn't find a single one. In old time APIs, some names starting with tm_ are reserved since they are in struct tm. This reservation is only implicit. Similarly for tv_ in struct timespec. POSIX is quite different. It reserves both tm_ and tv_. POSIX reserves so many prefixes and allows so much nested pollution that it is hard to know if the application namespace is nonempty. It reserves tv_ only in <time.h> and <sys/time.h>, but allows polluting many other headers with all of the symbols in these headers. FreeBSD doesn't extend struct timespec, so the implicit reservation of tv_sec and tv_nsec is all that is needed when struct timespec must be declared, and the ifdefs and organization don't need to be different for C11 to exclude tv_extension. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180919025337.F2618>