Hey Walter! and Jens!
On 10/21/21 10:13 AM, Walter Harms wrote:
Hi thx,
for the information, i was not aware of changes for the time interface
and i do a lot of programming with them.
Apparently, those changes finally did not make it into the standard (as
Jens pointed out to me). The reason was C++-compatibility.
Since you ask for it:
I do not like the [[gnu::nonnull]] as shown below.
The position triggers the wrong assoziation for me.
Things in front of a function are used to describe a return values.
I just checked the valid syntax accepted by the standard:
[
6.7 Declarations
[...]
Semantics
[...]
9 Except where specified otherwise, the meaning of an attribute
declaration is implementation-defined.
10 EXAMPLE In the declaration for an entity, attributes appertaining to
that entity may appear at the start of the declarationand after the
identifier for that declaration.
[[deprecated]] void f [[deprecated]] (void); // valid
Forward references: declarators (6.7.6), enumeration specifiers
(6.7.2.2), initialization (6.7.9), typenames (6.7.7), type qualifiers
(6.7.3).
]
So we could put it in any of those 2 positions. Is there any reason
that one is better (more readable / less ambiguous) than the other? We
should decide very carefully which one to use.
BTW, this example reminds me of the deprecated functions, for which I'm
going to add [[deprecated]] to clearly mark those. That's something
that had been itching me for a long time :).
To be fair the array solution is not great either.
my idea would to add a comment like
char *asctime(const struct tm *" tm /* not null */);
I think the C language is more universal than comments. If we can use
standard features of the language (or even GNU extensions, like in this
case), that will be readable across anyone that understands C or GNU C.
Comments are very personal, and do not have a universal meaning, and
that's why I try to avoid them in the man-pages SYNOPSIS as much as I
can. Apart from that, a comment after each parameter would be much more
noisy than a single [[gnu::nonnull]].
What is happening inside time.h is something different.
I guess you mean in the implementation side, right? time/mktime.c for
example.
If you thing the compiler should check for not null
he needs a hint.
IMHO it is the responsibility of the programmer
to make sure that the propper arguments are provided.
Sure, and that's exactly the reason to add these attributes to the
prototype. To tell the programmer that NULL is *not* a proper argument.
I don't consider these attributes as optimization opportunities for
the compiler (which they are, BTW), but mainly as documentation for the
user of the API. Since the man-pages are one of the most used
documentation sources for C functions, I think documenting these
attributes makes a lot of sense.
But yes, we should think very carefully the position in which we should
put them; especially since many programmers will not read the standard,
and instead just imitate what they see in the manual pages, so we better
use the least ambiguous syntax possible.
Jens, since IIRC it was you who added attributes to C2X, could you
please help us deciding between the two options?
Thanks!
Alex
re,
wh
________________________________________
Von: Alejandro Colomar <alx.manpages@xxxxxxxxx>
Gesendet: Mittwoch, 20. Oktober 2021 22:22:41
An: mtk.manpages@xxxxxxxxx
Cc: Alejandro Colomar; linux-man@xxxxxxxxxxxxxxx; Jens Gustedt; Glibc
Betreff: [PATCH 2/2] ctime.3, strftime.3, strptime.3, timegm.3: Add [[gnu::nonnull]] to <time.h> prototypes
WARNUNG: Diese E-Mail kam von außerhalb der Organisation. Klicken Sie nicht auf Links oder öffnen Sie keine Anhänge, es sei denn, Sie kennen den/die Absender*in und wissen, dass der Inhalt sicher ist.
C2X changes the prototypes of <time.h> functions that accept a
pointer that cannot be NULL, to use 'static', which clearly
denotes that passing NULL is Undefined Behavior.
For example, 'time_t mktime(struct tm tm[static 1]);'.
This change is backwards compatible, since array notation is just
syntactic sugar for pointers, and the Undefined Behavior in case
of a pointer already existed (in the wording); it just wasn't
clear from the prototype itself.
However, that forces the use of VLA (array) notation for something
that is *not* an array. It is cofusing, probably too much for
some programmers not so familiar with the difference between an
array and a pointer, and that happens more than we would like.
Even for programmers that clearly know the difference between an
array and a pointer, this is at least misleading.
That happens because the standard lacks a 'nonnull' attribute, and
only has that (VLA) way of expressing what GCC can express with
'[[gnu::nonnull]]' (a.k.a. '__attribute__((__nonnull__))').
Expressing that NULL pointers shall invoke Undefined Behavior in
the prototype of a function is *way* more readable than having to
read through the whole manual page text, so ideally we should also
follow the standard idea of expressing that. But we can make use
of more advanced techniques such as the GCC attribute, which help
keep the information that those pointers are actually pointers and
not arrays.
From the 2 different attribute notations, let's use the "C++" one,
which will be part of the standard in C2X (unlike __attribute__),
and is also shorter, which helps keep the SYNOPSIS short (mostly
one-liner prototypes).
See <http://www.open-std.org/JTC1/SC22/WG14/www/docs/n2417.pdf>
Signed-off-by: Alejandro Colomar <alx.manpages@xxxxxxxxx>
Cc: Jens Gustedt <jens.gustedt@xxxxxxxx>
Cc: Glibc <libc-alpha@xxxxxxxxxxxxxx>
---
man3/ctime.3 | 26 +++++++++++++-------------
man3/strftime.3 | 1 +
man3/strptime.3 | 1 +
man3/timegm.3 | 4 ++--
4 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/man3/ctime.3 b/man3/ctime.3
index 0620741e9..42021a588 100644
--- a/man3/ctime.3
+++ b/man3/ctime.3
@@ -40,23 +40,23 @@ localtime_r \- transform date and time to broken-down time or ASCII
.nf
.B #include <time.h>
.PP
-.BI "char *asctime(const struct tm *" tm );
-.BI "char *asctime_r(const struct tm *restrict " tm ,
-.BI " char " buf "[static restrict 26]);"
+.BI "[[gnu::nonnull]] char *asctime(const struct tm *" tm );
+.BI "[[gnu::nonnull]] char *asctime_r(const struct tm *restrict " tm ,
+.BI " char " buf "[static restrict 26]);"
.PP
-.BI "char *ctime(const time_t *" timep );
-.BI "char *ctime_r(const time_t *restrict " timep ,
-.BI " char " buf "[static restrict 26]);"
+.BI "[[gnu::nonnull]] char *ctime(const time_t *" timep );
+.BI "[[gnu::nonnull]] char *ctime_r(const time_t *restrict " timep ,
+.BI " char " buf "[static restrict 26]);"
.PP
-.BI "struct tm *gmtime(const time_t *" timep );
-.BI "struct tm *gmtime_r(const time_t *restrict " timep ,
-.BI " struct tm *restrict " result );
+.BI "[[gnu::nonnull]] struct tm *gmtime(const time_t *" timep );
+.BI "[[gnu::nonnull]] struct tm *gmtime_r(const time_t *restrict " timep ,
+.BI " struct tm *restrict " result );
.PP
-.BI "struct tm *localtime(const time_t *" timep );
-.BI "struct tm *localtime_r(const time_t *restrict " timep ,
-.BI " struct tm *restrict " result );
+.BI "[[gnu::nonnull]] struct tm *localtime(const time_t *" timep );
+.BI "[[gnu::nonnull]] struct tm *localtime_r(const time_t *restrict " timep ,
+.BI " struct tm *restrict " result );
.PP
-.BI "time_t mktime(struct tm *" tm );
+.BI "[[gnu::nonnull]] time_t mktime(struct tm *" tm );
.fi
.PP
.RS -4
diff --git a/man3/strftime.3 b/man3/strftime.3
index a24ea720b..715b30edb 100644
--- a/man3/strftime.3
+++ b/man3/strftime.3
@@ -41,6 +41,7 @@ strftime \- format date and time
.nf
.B #include <time.h>
.PP
+.B [[gnu::nonnull]]
.BI "size_t strftime(char *restrict " s ", size_t " max ,
.BI " const char *restrict " format ,
.BI " const struct tm *restrict " tm );
diff --git a/man3/strptime.3 b/man3/strptime.3
index d6595d4bf..c1b334d87 100644
--- a/man3/strptime.3
+++ b/man3/strptime.3
@@ -36,6 +36,7 @@ strptime \- convert a string representation of time to a time tm structure
.BR "#define _XOPEN_SOURCE" " /* See feature_test_macros(7) */"
.B #include <time.h>
.PP
+.B [[gnu::nonnull]]
.BI "char *strptime(const char *restrict " s ", const char *restrict " format ,
.BI " struct tm *restrict " tm );
.fi
diff --git a/man3/timegm.3 b/man3/timegm.3
index b848e83e1..18b6e4847 100644
--- a/man3/timegm.3
+++ b/man3/timegm.3
@@ -29,8 +29,8 @@ timegm, timelocal \- inverses of gmtime and localtime
.nf
.B #include <time.h>
.PP
-.BI "time_t timelocal(struct tm *" tm );
-.BI "time_t timegm(struct tm *" tm );
+.BI "[[gnu::nonnull]] time_t timelocal(struct tm *" tm );
+.BI "[[gnu::nonnull]] time_t timegm(struct tm *" tm );
.PP
.fi
.RS -4
--
2.33.0
--
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/
