Patchwork [libfortran] PR 47802 Implementation of CTIME intrinsic

login
register
mail settings
Submitter Janne Blomqvist
Date Feb. 24, 2011, 9:24 p.m.
Message ID <AANLkTikYQ2jrxtno4fsgEBEbrAhvHoREqm6RnrVKTw5F@mail.gmail.com>
Download mbox | patch
Permalink /patch/84488/
State New
Headers show

Comments

Janne Blomqvist - Feb. 24, 2011, 9:24 p.m.
On Wed, Feb 23, 2011 at 20:23, Tobias Burnus
<tobias.burnus@physik.fu-berlin.de> wrote:
> Janne Blomqvist wrote:
>> The attached patch changes the implementation of CTIME and FDATE to
>> use strftime instead.

>  fdate_sub (char * date, gfc_charlen_type date_len)
> [...]
> +  fctime (date, date_len, &now);
>
> As convenient as I find that one directly passes the buffer:
> Doesn't this reduce the number of characters effectively
> by one? Assume 'Sat Aug 19 18:13:14 1995'. That's 24 characters.
> If one now does:
>  character(len=24) :: str
>  call fdate(str)
> will this work? I fear that this will produce
> 'Sat Aug 19 18:13:14 199\0'. It seems to work is the string is
> at least one character longer than needed as then memset should
> get rid of the tailing '\0'.
>
>
> +   not available, gmtime use thread-local storage so it's
>
> s/use/uses/
>
>
>
> libgfortran/intrinsics/time_1.h
> +static struct tm *
> +localtime_r (const time_t * timep, struct tm * result)
> +{
> +  *result = *localtime (timep);
> +  return result;
> +}
>
> Any reason that you do not mark it as inline?

Patch with the above improvements attached. Ok for trunk?

2011-02-24  Janne Blomqvist  <jb@gcc.gnu.org>

	PR libfortran/47802
	* config.h.in: Regenerated.
	* configure: Regenerated.
	* configure.ac: Remove checks for ctime and ctime_r, add check for
	strftime.
	* intrinsics/date_and_time.c (localtime_r): Move fallback
	implementation to time_1.h.
	* intrinsics/time_1.h (localtime_r): Fallback implementation.
	* intrinsics/ctime.c: Include time_1.h.
	(ctime_r): Remove fallback implementation.
	(strctime): New function.
	(fdate): Use strctime instead of ctime_r.
	(fdate_sub): Likewise.
	(ctime): Likewise.
	(ctime_sub): Likewise.
Tobias Burnus - Feb. 24, 2011, 9:43 p.m.
Dear Janne,

I just wanted to reply to your earlier email. Now, I reply to both :-)

Janne Blomqvist wrote:
>>   fdate_sub (char * date, gfc_charlen_type date_len)
>> [...]
>> +  fctime (date, date_len,&now);
>>
>> As convenient as I find that one directly passes the buffer:
>> Doesn't this reduce the number of characters effectively
>> by one?
> Yes. Is this worth fixing? It would be easy to fix by using malloc to
> allocate a tmp array of size date_len + 1, and then call f_strcpy to
> copy the result. As the fix is easy, I'm thinking it should be done,
> but I have no particularly hard opinion.

(Is now fixed, but I still wanted to state why I think it makes sense.)

I think one should handle the case character(len=24) as that's the most 
obvious character length (count yourself!) and is the length which other 
Fortran implementations use - for the function ctime. Supporting ctime 
as subroutine seems to be the exception (g77 supports it). The length of 
24 is explicitly mentioned (for the function) by Sun/Oracle [1] and 
implicitly in the example by Intel [2]. (gfortran uses a length of 30 in 
its example [3].)

[1] http://download.oracle.com/docs/cd/E19205-01/819-5259/aetij/index.html
[2] 
http://software.intel.com/sites/products/documentation/hpc/compilerpro/en-us/fortran/mac/compiler_f/lref_for/source_files/rfctime.htm
[3] http://gcc.gnu.org/onlinedocs/gfortran/CTIME.html


By the way, in the documentation patch: Can one remove the ", (not 
recommended)." part after the function version? It is not obvious to me 
why the subroutine is better. g77 did not have this wording and as 
written other compilers have only the function version, which makes the 
function "better".

>> Assume 'Sat Aug 19 18:13:14 1995'. That's 24 characters.
>> If one now does:
>>   character(len=24) :: str
>>   call fdate(str)
>> will this work? I fear that this will produce
>> 'Sat Aug 19 18:13:14 199\0'.
> AFAICS it will produce a string filled with blanks.

One should write in the documentation that upon error (e.g. string too 
short) the result will be a blank string (subroutine) or a zero-sized 
string (function).

Janne Blomqvist wrote:
> Patch with the above improvements attached. Ok for trunk?

(I reply to the wrong patch, but I reviewed the one without memory leak ;-)


OK. Hopefully there are no surprises on rarer targets.

Thanks for the patch!

Tobias

> 2011-02-24  Janne Blomqvist<jb@gcc.gnu.org>
>
> 	PR libfortran/47802
> 	* config.h.in: Regenerated.
> 	* configure: Regenerated.
> 	* configure.ac: Remove checks for ctime and ctime_r, add check for
> 	strftime.
> 	* intrinsics/date_and_time.c (localtime_r): Move fallback
> 	implementation to time_1.h.
> 	* intrinsics/time_1.h (localtime_r): Fallback implementation.
> 	* intrinsics/ctime.c: Include time_1.h.
> 	(ctime_r): Remove fallback implementation.
> 	(strctime): New function.
> 	(fdate): Use strctime instead of ctime_r.
> 	(fdate_sub): Likewise.
> 	(ctime): Likewise.
> 	(ctime_sub): Likewise
Janne Blomqvist - Feb. 24, 2011, 10:02 p.m.
On Thu, Feb 24, 2011 at 23:43, Tobias Burnus <burnus@net-b.de> wrote:
> Dear Janne,
>
> I just wanted to reply to your earlier email. Now, I reply to both :-)
>
> Janne Blomqvist wrote:
>>>
>>>  fdate_sub (char * date, gfc_charlen_type date_len)
>>> [...]
>>> +  fctime (date, date_len,&now);
>>>
>>> As convenient as I find that one directly passes the buffer:
>>> Doesn't this reduce the number of characters effectively
>>> by one?
>>
>> Yes. Is this worth fixing? It would be easy to fix by using malloc to
>> allocate a tmp array of size date_len + 1, and then call f_strcpy to
>> copy the result. As the fix is easy, I'm thinking it should be done,
>> but I have no particularly hard opinion.
>
> (Is now fixed, but I still wanted to state why I think it makes sense.)
>
> I think one should handle the case character(len=24) as that's the most
> obvious character length (count yourself!) and is the length which other
> Fortran implementations use - for the function ctime. Supporting ctime as
> subroutine seems to be the exception (g77 supports it). The length of 24 is
> explicitly mentioned (for the function) by Sun/Oracle [1] and implicitly in
> the example by Intel [2]. (gfortran uses a length of 30 in its example [3].)
>
> [1] http://download.oracle.com/docs/cd/E19205-01/819-5259/aetij/index.html
> [2]
> http://software.intel.com/sites/products/documentation/hpc/compilerpro/en-us/fortran/mac/compiler_f/lref_for/source_files/rfctime.htm
> [3] http://gcc.gnu.org/onlinedocs/gfortran/CTIME.html

Good points.

> By the way, in the documentation patch: Can one remove the ", (not
> recommended)." part after the function version? It is not obvious to me why
> the subroutine is better. g77 did not have this wording and as written other
> compilers have only the function version, which makes the function "better".

I don't know why this recommendation was added, maybe it has to do
with functions with type character*(*) being confusing and error prone
in general?

>
>>> Assume 'Sat Aug 19 18:13:14 1995'. That's 24 characters.
>>> If one now does:
>>>  character(len=24) :: str
>>>  call fdate(str)
>>> will this work? I fear that this will produce
>>> 'Sat Aug 19 18:13:14 199\0'.
>>
>> AFAICS it will produce a string filled with blanks.
>
> One should write in the documentation that upon error (e.g. string too
> short) the result will be a blank string (subroutine) or a zero-sized string
> (function).

Yes, will do. Actually, it's even worse, in that in the implementation
of the function version we don't know the length of the result string,
so we assume 100 is enough (the CSZ constant) for the temporary
string, and then the frontend truncates if necessary when copying to
the result variable. I see no reason why the frontend in principle
couldn't pass the result string and its length to the function
version, but this is apparently how gfortran handles character*(*)
functions..

> OK. Hopefully there are no surprises on rarer targets.

Yes, indeed!

Thanks for the review, committed as r170478.

Patch

diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index d25a067..66468db 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -248,11 +248,11 @@  AC_CHECK_MEMBERS([struct stat.st_rdev])
 # Check for library functions.
 AC_CHECK_FUNCS(getrusage times mkstemp strtof strtold snprintf ftruncate chsize)
 AC_CHECK_FUNCS(chdir strerror getlogin gethostname kill link symlink perror)
-AC_CHECK_FUNCS(sleep time ttyname signal alarm ctime clock access fork execl)
+AC_CHECK_FUNCS(sleep time ttyname signal alarm clock access fork execl)
 AC_CHECK_FUNCS(wait setmode execvp pipe dup2 close fdopen strcasestr getrlimit)
 AC_CHECK_FUNCS(gettimeofday stat fstat lstat getpwuid vsnprintf dup getcwd)
-AC_CHECK_FUNCS(localtime_r gmtime_r strerror_r getpwuid_r ttyname_r ctime_r)
-AC_CHECK_FUNCS(clock_gettime)
+AC_CHECK_FUNCS(localtime_r gmtime_r strerror_r getpwuid_r ttyname_r)
+AC_CHECK_FUNCS(clock_gettime strftime)
 
 # Check for glibc backtrace functions
 AC_CHECK_FUNCS(backtrace backtrace_symbols)
diff --git a/libgfortran/intrinsics/ctime.c b/libgfortran/intrinsics/ctime.c
index b7b463c..932514e 100644
--- a/libgfortran/intrinsics/ctime.c
+++ b/libgfortran/intrinsics/ctime.c
@@ -25,42 +25,31 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 
 #include "libgfortran.h"
 
-#ifdef TIME_WITH_SYS_TIME
-#  include <sys/time.h>
-#  include <time.h>
-#else
-#  if HAVE_SYS_TIME_H
-#    include <sys/time.h>
-#  else
-#    ifdef HAVE_TIME_H
-#      include <time.h>
-#    endif
-#  endif
-#endif
+#include "time_1.h"
 
 #include <string.h>
 
 
-#ifndef HAVE_CTIME_R
-/* Make sure we don't see here a macro.  */
-#undef ctime_r
+/* strftime-like function that fills a C string with %c format which
+   is identical to ctime in the default locale. As ctime and ctime_r
+   are poorly specified and their usage not recommended, the
+   implementation instead uses strftime.  */
 
-static char *
-ctime_r (const time_t * timep, char * buf __attribute__((unused)))
+static size_t
+strctime (char *s, size_t max, const time_t *timep)
 {
-#ifdef HAVE_CTIME
-  char *tmp = ctime (timep);
-  if (tmp)
-    tmp = strcpy (buf, tmp);
-  return tmp;
+#ifdef HAVE_STRFTIME
+  struct tm res;
+  struct tm *ltm = localtime_r (timep, &res);
+  return strftime (s, max, "%c", ltm);
 #else
-  return NULL;
+  return 0;
 #endif
 }
-#endif
 
-/* ctime_r() buffer size needs to be at least 26 bytes.  */
-#define CSZ 26
+/* In the default locale, the date and time representation fits in 26
+   bytes. However, other locales might need more space.  */
+#define CSZ 100
 
 extern void fdate (char **, gfc_charlen_type *);
 export_proto(fdate);
@@ -68,29 +57,15 @@  export_proto(fdate);
 void
 fdate (char ** date, gfc_charlen_type * date_len)
 {
-#if defined(HAVE_TIME) && defined(HAVE_CTIME)
-  char cbuf[CSZ];
-  int i;
+#if defined(HAVE_TIME)
   time_t now = time(NULL);
-  *date = ctime_r (&now, cbuf);
-  if (*date != NULL)
-    {
-      *date = strdup (*date);
-      *date_len = strlen (*date);
-
-      i = 0;
-      while ((*date)[i])
-       {
-         if ((*date)[i] == '\n')
-           (*date)[i] = ' ';
-         i++;
-       }
-      return;
-    }
-#endif
+  *date = get_mem (CSZ);
+  *date_len = strctime (*date, CSZ, &now);
+#else
 
   *date = NULL;
   *date_len = 0;
+#endif
 }
 
 
@@ -100,22 +75,14 @@  export_proto(fdate_sub);
 void
 fdate_sub (char * date, gfc_charlen_type date_len)
 {
-#if defined(HAVE_TIME) && defined(HAVE_CTIME)
-  char cbuf[CSZ];
-  int i;
-  char *d;
+#if defined(HAVE_TIME)
   time_t now = time(NULL);
-#endif
-  
+  char *s = get_mem (date_len + 1);
+  size_t n = strctime (s, date_len + 1, &now);
+  fstrcpy (date, date_len, s, n);
+  free (s);
+#else
   memset (date, ' ', date_len);
-#if defined(HAVE_TIME) && defined(HAVE_CTIME)
-  d = ctime_r (&now, cbuf);
-  if (d != NULL)
-    {
-      i = 0;
-      while (*d && *d != '\n' && i < date_len)
-       date[i++] = *(d++);
-    }
 #endif
 }
 
@@ -127,29 +94,15 @@  export_proto_np(PREFIX(ctime));
 void
 PREFIX(ctime) (char ** date, gfc_charlen_type * date_len, GFC_INTEGER_8 t)
 {
-#if defined(HAVE_CTIME)
-  char cbuf[CSZ];
+#if defined(HAVE_TIME)
   time_t now = t;
-  int i;
-  *date = ctime_r (&now, cbuf);
-  if (*date != NULL)
-    {
-      *date = strdup (*date);
-      *date_len = strlen (*date);
-
-      i = 0;
-      while ((*date)[i])
-       {
-         if ((*date)[i] == '\n')
-           (*date)[i] = ' ';
-         i++;
-       }
-      return;
-    }
-#endif
+  *date = get_mem (CSZ);
+  *date_len = strctime (*date, CSZ, &now);
+#else
 
   *date = NULL;
   *date_len = 0;
+#endif
 }
 
 
@@ -159,21 +112,8 @@  export_proto(ctime_sub);
 void
 ctime_sub (GFC_INTEGER_8 * t, char * date, gfc_charlen_type date_len)
 {
-#if defined(HAVE_CTIME)
-  char cbuf[CSZ];
-  int i;
-  char *d;
   time_t now = *t;
-#endif
-  
-  memset (date, ' ', date_len);
-#if defined(HAVE_CTIME)
-  d = ctime_r (&now, cbuf);
-  if (d != NULL)
-    {
-      i = 0;
-      while (*d && *d != '\n' && i < date_len)
-       date[i++] = *(d++);
-    }
-#endif
+  char *s = get_mem (date_len + 1);
+  size_t n = strctime (s, date_len + 1, &now);
+  fstrcpy (date, date_len, s, n);
 }
diff --git a/libgfortran/intrinsics/date_and_time.c b/libgfortran/intrinsics/date_and_time.c
index c58d114..793df68 100644
--- a/libgfortran/intrinsics/date_and_time.c
+++ b/libgfortran/intrinsics/date_and_time.c
@@ -36,24 +36,10 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #endif
 
 
-/* If the re-entrant versions of localtime and gmtime are not
-   available, provide fallback implementations.  On some targets where
-   the _r versions are not available, localtime and gmtime use
-   thread-local storage so they are threadsafe.  */
-
-#ifndef HAVE_LOCALTIME_R
-/* If _POSIX is defined localtime_r gets defined by mingw-w64 headers.  */
-#ifdef localtime_r
-#undef localtime_r
-#endif
-
-static struct tm *
-localtime_r (const time_t * timep, struct tm * result)
-{
-  *result = *localtime (timep);
-  return result;
-}
-#endif
+/* If the re-entrant version of gmtime is not available, provide a
+   fallback implementation.  On some targets where the _r version is
+   not available, gmtime uses thread-local storage so it's
+   threadsafe.  */
 
 #ifndef HAVE_GMTIME_R
 /* If _POSIX is defined gmtime_r gets defined by mingw-w64 headers.  */
diff --git a/libgfortran/intrinsics/time_1.h b/libgfortran/intrinsics/time_1.h
index 073595a..12d79eb 100644
--- a/libgfortran/intrinsics/time_1.h
+++ b/libgfortran/intrinsics/time_1.h
@@ -84,6 +84,26 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #endif  /* !HAVE_GETRUSAGE || !HAVE_SYS_RESOURCE_H  */
 
 
+/* If the re-entrant version of localtime is not available, provide a
+   fallback implementation.  On some targets where the _r version is
+   not available, localtime uses thread-local storage so it's
+   threadsafe.  */
+
+#ifndef HAVE_LOCALTIME_R
+/* If _POSIX is defined localtime_r gets defined by mingw-w64 headers.  */
+#ifdef localtime_r
+#undef localtime_r
+#endif
+
+static inline struct tm *
+localtime_r (const time_t * timep, struct tm * result)
+{
+  *result = *localtime (timep);
+  return result;
+}
+#endif
+
+
 #if defined (__GNUC__) && (__GNUC__ >= 3)
 #  define ATTRIBUTE_ALWAYS_INLINE __attribute__ ((__always_inline__))
 #else