[libfortran] PR 47802 Implementation of CTIME intrinsic

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.

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

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.