Patchwork [03/12] Don’t use the argument to time.

login
register
mail settings
Submitter Zack Weinberg
Date Aug. 20, 2019, 1:21 p.m.
Message ID <20190820132152.24100-4-zackw@panix.com>
Download mbox | patch
Permalink /patch/34183/
State New
Headers show

Comments

Zack Weinberg - Aug. 20, 2019, 1:21 p.m.
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(-)
Rafal Luzynski - Aug. 20, 2019, 5:08 p.m.
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.
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.
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 Netto - Aug. 20, 2019, 6:08 p.m.
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.
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.
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.
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.
* 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.
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.
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.
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.)
Rafal Luzynski - Aug. 23, 2019, 10:06 p.m.
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.
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.
Stefan Liebler - Aug. 26, 2019, 2:42 p.m.
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

Patch

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]),