Skip site navigation (1)Skip section navigation (2)
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>