[03/12] Don’t use the argument to time.
diff mbox series

Message ID 20190820132152.24100-4-zackw@panix.com
State New
Headers show
Series
  • Y2038 preparation: use clock_[gs]ettime to implement the other time-getting and -setting functions
Related show

Commit Message

Zack Weinberg Aug. 20, 2019, 1:21 p.m. UTC
Unlike gettimeofday, I don’t think it makes sense to remove all the
internal uses of time.  Its callers don’t care about sub-second
resolution and would be unnecessarily complicated if they had to
declare a struct timespec instead of just a time_t.  However, a
handful of places were using the vestigial ‘result’ argument instead
of the return value, which is ever so slightly less efficient and also
looks weird.  Correct this.

	* misc/syslog.c (__vsyslog_internal)
	* time/getdate.c (__getdate_r)
	* time/tst_wcsftime.c (main):
	Use return value of time, not its argument.

	* string/strfry.c (strfry)
	* sysdeps/mach/sleep.c (__sleep):
	Remove unnecessary casts of NULL.
---
 misc/syslog.c        | 2 +-
 string/strfry.c      | 2 +-
 sysdeps/mach/sleep.c | 4 ++--
 time/getdate.c       | 2 +-
 time/tst_wcsftime.c  | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

Comments

Rafal Luzynski Aug. 20, 2019, 5:08 p.m. UTC | #1
Zack, I think that your email software replaces the plain ASCII
apostrophe character "'" (ASCII/Unicode 0x0027) with the right single
quotation mark character "’" (Unicode 0x2019).  I am not sure if
we want this in the git comments if possible to keep the plain
ASCII.  This applies to other patches as well.

Please disregard this message if this is not a problem.

20.08.2019 15:21 Zack Weinberg <zackw@panix.com> wrote:
> 
> Unlike gettimeofday, I don’t think it makes sense to remove all the

Here is an example: --------^

Regards,

Rafal
Zack Weinberg Aug. 20, 2019, 5:21 p.m. UTC | #2
On Tue, Aug 20, 2019 at 1:09 PM Rafal Luzynski
<digitalfreak@lingonborough.com> wrote:
> Zack, I think that your email software replaces the plain ASCII
> apostrophe character "'" (ASCII/Unicode 0x0027) with the right single
> quotation mark character "’" (Unicode 0x2019).  I am not sure if
> we want this in the git comments if possible to keep the plain
> ASCII.  This applies to other patches as well.

That was actually my text editor when I wrote the commit messages in
the first place, but it's a fair question anyway.  What _is_ our
policy on non-ASCII characters in general, and "smart" / "typographic"
quote marks in particular, in commit messages and ChangeLogs?

zw
Joseph Myers Aug. 20, 2019, 5:35 p.m. UTC | #3
On Tue, 20 Aug 2019, Rafal Luzynski wrote:

> Zack, I think that your email software replaces the plain ASCII
> apostrophe character "'" (ASCII/Unicode 0x0027) with the right single
> quotation mark character "’" (Unicode 0x2019).  I am not sure if
> we want this in the git comments if possible to keep the plain
> ASCII.  This applies to other patches as well.

Clearly such characters should be allowed in the names of committers, for 
example.  We have the known and previously discussed bug in the commit 
hooks that they mark glibc-cvs messages as text/plain; charset="us-ascii" 
even when that's inaccurate, but I think that should be fixed in the 
hooks.
Adhemerval Zanella Aug. 20, 2019, 6:08 p.m. UTC | #4
On 20/08/2019 10:21, Zack Weinberg wrote:
> Unlike gettimeofday, I don’t think it makes sense to remove all the
> internal uses of time.  Its callers don’t care about sub-second
> resolution and would be unnecessarily complicated if they had to
> declare a struct timespec instead of just a time_t.  However, a
> handful of places were using the vestigial ‘result’ argument instead
> of the return value, which is ever so slightly less efficient and also
> looks weird.  Correct this.
> 
> 	* misc/syslog.c (__vsyslog_internal)
> 	* time/getdate.c (__getdate_r)
> 	* time/tst_wcsftime.c (main):
> 	Use return value of time, not its argument.
> 
> 	* string/strfry.c (strfry)
> 	* sysdeps/mach/sleep.c (__sleep):
> 	Remove unnecessary casts of NULL.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  misc/syslog.c        | 2 +-
>  string/strfry.c      | 2 +-
>  sysdeps/mach/sleep.c | 4 ++--
>  time/getdate.c       | 2 +-
>  time/tst_wcsftime.c  | 2 +-
>  5 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/misc/syslog.c b/misc/syslog.c
> index 3a15da41ce..cf2deef533 100644
> --- a/misc/syslog.c
> +++ b/misc/syslog.c
> @@ -205,7 +205,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
>  	  {
>  	    __fsetlocking (f, FSETLOCKING_BYCALLER);
>  	    fprintf (f, "<%d>", pri);
> -	    (void) time (&now);
> +	    now = time (NULL);
>  	    f->_IO_write_ptr += __strftime_l (f->_IO_write_ptr,
>  					      f->_IO_write_end
>  					      - f->_IO_write_ptr,

Ok.

> diff --git a/string/strfry.c b/string/strfry.c
> index af6087bee5..71686d45c2 100644
> --- a/string/strfry.c
> +++ b/string/strfry.c
> @@ -30,7 +30,7 @@ strfry (char *string)
>      {
>        static char state[32];
>        rdata.state = NULL;
> -      __initstate_r (time ((time_t *) NULL) ^ getpid (),
> +      __initstate_r (time (NULL) ^ getpid (),
>  		     state, sizeof (state), &rdata);
>        init = 1;
>      }

Ok.

> diff --git a/sysdeps/mach/sleep.c b/sysdeps/mach/sleep.c
> index 11e1bb87f3..c63ef926b7 100644
> --- a/sysdeps/mach/sleep.c
> +++ b/sysdeps/mach/sleep.c
> @@ -33,10 +33,10 @@ __sleep (unsigned int seconds)
>  
>    recv = __mach_reply_port ();
>  
> -  before = time ((time_t *) NULL);
> +  before = time (NULL);
>    (void) __mach_msg (NULL, MACH_RCV_MSG|MACH_RCV_TIMEOUT|MACH_RCV_INTERRUPT,
>  		     0, 0, recv, seconds * 1000, MACH_PORT_NULL);
> -  after = time ((time_t *) NULL);
> +  after = time (NULL);
>    __mach_port_destroy (__mach_task_self (), recv);
>  
>    return seconds - (after - before);

Ok.

> diff --git a/time/getdate.c b/time/getdate.c
> index aee96f7163..8a567c3fcd 100644
> --- a/time/getdate.c
> +++ b/time/getdate.c
> @@ -219,7 +219,7 @@ __getdate_r (const char *string, struct tm *tp)
>      return 7;
>  
>    /* Get current time.  */
> -  time (&timer);
> +  timer = time (NULL);
>    __localtime_r (&timer, &tm);
>  
>    /* If only the weekday is given, today is assumed if the given day

Ok.

> diff --git a/time/tst_wcsftime.c b/time/tst_wcsftime.c
> index 3f6f0d9f77..55c45f6a81 100644
> --- a/time/tst_wcsftime.c
> +++ b/time/tst_wcsftime.c
> @@ -10,7 +10,7 @@ main (int argc, char *argv[])
>    int result = 0;
>    size_t n;
>  
> -  time (&t);
> +  t = time (NULL);
>    tp = gmtime (&t);
>  
>    n = wcsftime (buf, sizeof (buf) / sizeof (buf[0]),
> 

Ok.
Zack Weinberg Aug. 21, 2019, 12:29 p.m. UTC | #5
On Tue, Aug 20, 2019 at 2:08 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> >       * misc/syslog.c (__vsyslog_internal)
> >       * time/getdate.c (__getdate_r)
> >       * time/tst_wcsftime.c (main):
> >       Use return value of time, not its argument.
> >
> >       * string/strfry.c (strfry)
> >       * sysdeps/mach/sleep.c (__sleep):
> >       Remove unnecessary casts of NULL.
>
> LGTM, thanks.
>
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

Committed, thanks.

I removed all of the non-ASCII characters from the commit message to
avoid tangling these changes up with an unrelated policy question.

zw
Rafal Luzynski Aug. 22, 2019, 9:37 a.m. UTC | #6
21.08.2019 14:29 Zack Weinberg <zackw@panix.com> wrote:
> [...]
> I removed all of the non-ASCII characters from the commit message to
> avoid tangling these changes up with an unrelated policy question.

Thank you for this.

Regards,

Rafal
Rafal Luzynski Aug. 22, 2019, 9:42 a.m. UTC | #7
20.08.2019 19:35 Joseph Myers <joseph@codesourcery.com> wrote:
> [...]
> Clearly such characters should be allowed in the names of committers, for 
> example.  We have the known and previously discussed bug in the commit 
> hooks that they mark glibc-cvs messages as text/plain; charset="us-ascii" 
> even when that's inaccurate, but I think that should be fixed in the 
> hooks.

I agree, I personally suffer from the same problem.  In fact, I have
3 non-ASCII characters in my name but long ago but I replace them with
ASCII characters even if computers nowadays are able to handle Unicode
(as we can see, that is not always true).  I would be happy if we could
allow all Unicode characters, at least extended Latin, my point is that
probably we have not yet achieved this ability.

Regards,

Rafal
Florian Weimer Aug. 22, 2019, 9:49 a.m. UTC | #8
* Rafal Luzynski:

> 20.08.2019 19:35 Joseph Myers <joseph@codesourcery.com> wrote:
>> [...]
>> Clearly such characters should be allowed in the names of committers, for 
>> example.  We have the known and previously discussed bug in the commit 
>> hooks that they mark glibc-cvs messages as text/plain; charset="us-ascii" 
>> even when that's inaccurate, but I think that should be fixed in the 
>> hooks.
>
> I agree, I personally suffer from the same problem.  In fact, I have
> 3 non-ASCII characters in my name but long ago but I replace them with
> ASCII characters even if computers nowadays are able to handle Unicode
> (as we can see, that is not always true).  I would be happy if we could
> allow all Unicode characters, at least extended Latin, my point is that
> probably we have not yet achieved this ability.

Many of us have committed patches with author names which contain
non-ASCII characters.  Using UTF-8 quotation marks in GCC error messages
in commit messages is common, too.  I think the issues we saw with
commit notifications are really minor.

So please feel free to spell your name in the way you want.

Thanks,
Florian
Paul Eggert Aug. 23, 2019, 8:26 p.m. UTC | #9
On 8/22/19 2:49 AM, Florian Weimer wrote:
> please feel free to spell your name in the way you want.

Yes, I went through Emacs ChangeLog files a while ago and fixed many spellings 
of names that had been ASCIIfied under the assumption that it was problematic to 
put non-UTF8 characters in text files. It's only polite for us to spell our 
contributors' names correctly, so I installed the attached patch to try to do a 
better job with glibc contributor names too. Because the patch has mixed 
encodings I compressed it; you may need special settings to read it, but it 
works fine with Git.

A few of our recent contributors (Rafael Ávila de Espíndola, Uroš Bizjak, 
Alexandra Hájková, István Kurucsai, Rafał Łużyński) use their natively-spelled 
names sometimes and their ASCIIfied names at other times, and it's conceivable 
that they prefer their names to be ASCIIfied when used in English text so I'll 
ask them before adjusting their names in the glibc commentary.
Joseph Myers Aug. 23, 2019, 8:48 p.m. UTC | #10
On Fri, 23 Aug 2019, Paul Eggert wrote:

> Yes, I went through Emacs ChangeLog files a while ago and fixed many spellings
> of names that had been ASCIIfied under the assumption that it was problematic
> to put non-UTF8 characters in text files. It's only polite for us to spell our
> contributors' names correctly, so I installed the attached patch to try to do
> a better job with glibc contributor names too. Because the patch has mixed
> encodings I compressed it; you may need special settings to read it, but it
> works fine with Git.

This patch changes several *installed* MIPS headers.  As per 
<https://sourceware.org/ml/libc-alpha/2019-03/msg00293.html> and thread, 
we need to limit installed headers to pure ASCII so they work for 
compilations with any -finput-charset= setting.  So please revert the 
changes to sysdeps/mips/fpu_control.h, sysdeps/mips/regdef.h, 
sysdeps/mips/sgidefs.h, sysdeps/mips/sys/asm.h, sysdeps/mips/sys/regdef.h 
and sysdeps/unix/sysv/linux/mips/sys/user.h, or add the contributors in 
question to contrib.texi and remove the Contributed by lines from the 
headers.
Joseph Myers Aug. 23, 2019, 8:51 p.m. UTC | #11
On Fri, 23 Aug 2019, Joseph Myers wrote:

> and sysdeps/unix/sysv/linux/mips/sys/user.h, or add the contributors in 
> question to contrib.texi and remove the Contributed by lines from the 
> headers.

(While we want to move away from Contributed by lines and put those 
credits in contrib.texi instead, removal isn't an option when the name 
appears in a copyright notice rather than a Contributed by line, which 
applies in at least one case.)
Paul Eggert Aug. 23, 2019, 9:28 p.m. UTC | #12
Joseph Myers wrote:
> (While we want to move away from Contributed by lines and put those
> credits in contrib.texi instead, removal isn't an option when the name
> appears in a copyright notice rather than a Contributed by line, which
> applies in at least one case.)

Yes, thanks for mentioning these glitches. I installed the attached to fix them.

I'm afraid we're stuck with the ASCIIfied spellings in the old copyright notice 
in sysdeps/unix/sysv/linux/mips/sys/user.h, as 
<https://sourceware.org/ml/libc-alpha/2019-03/msg00293.html> says the file must 
be ASCII, and the file's copyright notice requires that we keep the 
contributors' names. This is merely an annoyance of course, and I assume any 
future contributors in this situation won't mind ASCIIfying their names either.
Rafal Luzynski Aug. 23, 2019, 10:06 p.m. UTC | #13
23.08.2019 22:26 Paul Eggert <eggert@cs.ucla.edu> wrote:
> [...]
> A few of our recent contributors (Rafael Ávila de Espíndola, Uroš Bizjak, 
> Alexandra Hájková, István Kurucsai, Rafał Łużyński) use their
> natively-spelled 
> names sometimes and their ASCIIfied names at other times, and it's
> conceivable 
> that they prefer their names to be ASCIIfied when used in English text so
> I'll 
> ask them before adjusting their names in the glibc commentary.

I have already replied off-list but now I think it should be said in public:
my name is misspelled here so please don't take it as a correct example.

Now one more issue: do I assume correctly that this idea is limited only to
Latin alphabets?  What about Cyrillic or Greek?  What about Asian alphabets?

Regards,

Rafał
Paul Eggert Aug. 23, 2019, 10:31 p.m. UTC | #14
Rafal Luzynski wrote:
> my name is misspelled here so please don't take it as a correct example.

Yes, sorry about that; as I mentioned privately, I got the wrong spelling by 
mistakenly lifting it from a web page about someone else who I thought was you 
but who spells their name with different accents.

> Now one more issue: do I assume correctly that this idea is limited only to
> Latin alphabets?

Yes, the glibc source code is in English so its text should use spelling 
preferred for an English-language audience. Although the audience can't grok 
names written in Cyrillic, Greek, Kanji, etc., it can grok Latin letters with 
accents.
Florian Weimer Aug. 24, 2019, 5:58 a.m. UTC | #15
* Paul Eggert:

> Joseph Myers wrote:
>> (While we want to move away from Contributed by lines and put those
>> credits in contrib.texi instead, removal isn't an option when the name
>> appears in a copyright notice rather than a Contributed by line, which
>> applies in at least one case.)
>
> Yes, thanks for mentioning these glitches. I installed the attached
> to fix them.
>
> I'm afraid we're stuck with the ASCIIfied spellings in the old
> copyright notice in sysdeps/unix/sysv/linux/mips/sys/user.h, as
> <https://sourceware.org/ml/libc-alpha/2019-03/msg00293.html> says
> the file must be ASCII, and the file's copyright notice requires
> that we keep the contributors' names. This is merely an annoyance of
> course, and I assume any future contributors in this situation won't
> mind ASCIIfying their names either.

Please restore the Contributed By lines as well.  We should not
discriminate against authors based on the preferred spelling of their
names.
Paul Eggert Aug. 25, 2019, 12:56 a.m. UTC | #16
Florian Weimer wrote:
> Please restore the Contributed By lines as well.  We should not
> discriminate against authors based on the preferred spelling of their
> names.

I don't see that discrimination, as the policy (as I understand it) is that 
"Contributed by" is obsolete and it's fine to move the information involved to 
contrib.texi whenever convenient. If I'm understanding the policy incorrectly, 
please let me know.

It wouldn't be hard to move all the "Contributed by" info to contrib.texi. Would 
that suffice to allay any concerns about discrimination?
Stefan Liebler Aug. 26, 2019, 2:42 p.m. UTC | #17
On 8/23/19 10:26 PM, Paul Eggert wrote:
> On 8/22/19 2:49 AM, Florian Weimer wrote:
>> please feel free to spell your name in the way you want.
> 
> Yes, I went through Emacs ChangeLog files a while ago and fixed many 
> spellings of names that had been ASCIIfied under the assumption that it 
> was problematic to put non-UTF8 characters in text files. It's only 
> polite for us to spell our contributors' names correctly, so I installed 
> the attached patch to try to do a better job with glibc contributor 
> names too. Because the patch has mixed encodings I compressed it; you 
> may need special settings to read it, but it works fine with Git.
> 
> A few of our recent contributors (Rafael Ávila de Espíndola, Uroš 
> Bizjak, Alexandra Hájková, István Kurucsai, Rafał Łużyński) use their 
> natively-spelled names sometimes and their ASCIIfied names at other 
> times, and it's conceivable that they prefer their names to be ASCIIfied 
> when used in English text so I'll ask them before adjusting their names 
> in the glibc commentary.

Hi,

after this change, the testcase posix/tst-regex is failing!
Please have a look at my proposed patch "[PATCH] Fix posix/tst-regex by 
using ISO-8859 encoding for ChangeLog.8." 
(https://www.sourceware.org/ml/libc-alpha/2019-08/msg00658.html)

Bye
Stefan
Joseph Myers Aug. 27, 2019, 3:58 p.m. UTC | #18
On Sat, 24 Aug 2019, Paul Eggert wrote:

> It wouldn't be hard to move all the "Contributed by" info to contrib.texi.
> Would that suffice to allay any concerns about discrimination?

I think it should all go in the manual.  (Where people already have 
entries in contrib.texi, those may or may not sufficiently cover all the 
areas in which they have contributed.)

*But* the information in the manual should generally describe the logical 
areas in which someone contributed, not the names of particular files.  
("contributions to the MIPS port", for example, would seem a better 
description in the case currently under discussion.)  The details at the 
per-file level are covered by the ChangeLog files.

Patch
diff mbox series

diff --git a/misc/syslog.c b/misc/syslog.c
index 3a15da41ce..cf2deef533 100644
--- a/misc/syslog.c
+++ b/misc/syslog.c
@@ -205,7 +205,7 @@  __vsyslog_internal(int pri, const char *fmt, va_list ap,
 	  {
 	    __fsetlocking (f, FSETLOCKING_BYCALLER);
 	    fprintf (f, "<%d>", pri);
-	    (void) time (&now);
+	    now = time (NULL);
 	    f->_IO_write_ptr += __strftime_l (f->_IO_write_ptr,
 					      f->_IO_write_end
 					      - f->_IO_write_ptr,
diff --git a/string/strfry.c b/string/strfry.c
index af6087bee5..71686d45c2 100644
--- a/string/strfry.c
+++ b/string/strfry.c
@@ -30,7 +30,7 @@  strfry (char *string)
     {
       static char state[32];
       rdata.state = NULL;
-      __initstate_r (time ((time_t *) NULL) ^ getpid (),
+      __initstate_r (time (NULL) ^ getpid (),
 		     state, sizeof (state), &rdata);
       init = 1;
     }
diff --git a/sysdeps/mach/sleep.c b/sysdeps/mach/sleep.c
index 11e1bb87f3..c63ef926b7 100644
--- a/sysdeps/mach/sleep.c
+++ b/sysdeps/mach/sleep.c
@@ -33,10 +33,10 @@  __sleep (unsigned int seconds)
 
   recv = __mach_reply_port ();
 
-  before = time ((time_t *) NULL);
+  before = time (NULL);
   (void) __mach_msg (NULL, MACH_RCV_MSG|MACH_RCV_TIMEOUT|MACH_RCV_INTERRUPT,
 		     0, 0, recv, seconds * 1000, MACH_PORT_NULL);
-  after = time ((time_t *) NULL);
+  after = time (NULL);
   __mach_port_destroy (__mach_task_self (), recv);
 
   return seconds - (after - before);
diff --git a/time/getdate.c b/time/getdate.c
index aee96f7163..8a567c3fcd 100644
--- a/time/getdate.c
+++ b/time/getdate.c
@@ -219,7 +219,7 @@  __getdate_r (const char *string, struct tm *tp)
     return 7;
 
   /* Get current time.  */
-  time (&timer);
+  timer = time (NULL);
   __localtime_r (&timer, &tm);
 
   /* If only the weekday is given, today is assumed if the given day
diff --git a/time/tst_wcsftime.c b/time/tst_wcsftime.c
index 3f6f0d9f77..55c45f6a81 100644
--- a/time/tst_wcsftime.c
+++ b/time/tst_wcsftime.c
@@ -10,7 +10,7 @@  main (int argc, char *argv[])
   int result = 0;
   size_t n;
 
-  time (&t);
+  t = time (NULL);
   tp = gmtime (&t);
 
   n = wcsftime (buf, sizeof (buf) / sizeof (buf[0]),