[m-rev.] for review: improve error checking in the time module

[Julien Fischer]

Hi Zoltan,

On Mon, 9 Aug 2021, Zoltan Somogyi wrote:

> On Sun, 8 Aug 2021 23:25:58 +1000 (AEST), Julien Fischer <jfischer at opturion.com> wrote:
>> @@ -205,7 +206,10 @@
>>           #include <unistd.h>
>>       #endif
>>
>> -    #include ""mercury_timing.h"" // for MR_CLOCK_TICKS_PER_SECOND
>> +    #include ""mercury_timing.h""       // for MR_CLOCK_TICKS_PER_SECOND
>> +    #include ""mercury_runtime_util.h"" // For MR_sterror.
>
> MR_stRerror

Fixed.

>>   localtime(Time, TM, !IO) :-
>>       Time = time_t(RawTime),
>> -    c_localtime(RawTime, Yr, Mnt, MD, Hrs, Min, Sec, YD, WD, N, !IO),
>> -    TM = tm(Yr, Mnt, MD, Hrs, Min, Sec, YD, WD, int_to_maybe_dst(N)).
>> +    c_localtime(RawTime, IsOk, Yr, Mnt, MD, Hrs, Min, Sec, YD, WD, N,
>> +        ErrorMsg, !IO),
>
> You may want to rename c_X to something like target_X, since it is
> a bit surprising to have a non-C foreign proc for something whose name
> implies it is specifically for C.

Done.

>> @@ -647,27 +704,37 @@ gmtime(time_t(Time)) = TM :-
>>       t = Time;
>>
>>       p = gmtime(&t);
>> -
>> -    // XXX do we need to handle the case where p == NULL here?
>> -
>> -    Sec = (MR_Integer) p->tm_sec;
>> -    Min = (MR_Integer) p->tm_min;
>> -    Hrs = (MR_Integer) p->tm_hour;
>> -    Mnt = (MR_Integer) p->tm_mon;
>> -    Yr = (MR_Integer) p->tm_year;
>> -    WD = (MR_Integer) p->tm_wday;
>> -    MD = (MR_Integer) p->tm_mday;
>> -    YD = (MR_Integer) p->tm_yday;
>> -    N = (MR_Integer) p->tm_isdst;
>> +    if (p == NULL) {
>> +        char errbuf[MR_STRERROR_BUF_SIZE];
>> +        const char *errno_msg;
>> +        IsOk = MR_NO;
>> +        errno_msg = MR_strerror(errno, errbuf, sizeof(errbuf));
>> +        MR_save_transient_hp();
>> +        MR_make_aligned_string_copy(ErrorMsg, errno_msg);
>> +        MR_restore_transient_hp();
>> +    } else {
>> +        IsOk = MR_YES;
>> +        Sec = (MR_Integer) p->tm_sec;
>> +        Min = (MR_Integer) p->tm_min;
>> +        Hrs = (MR_Integer) p->tm_hour;
>> +        Mnt = (MR_Integer) p->tm_mon;
>> +        Yr = (MR_Integer) p->tm_year;
>> +        WD = (MR_Integer) p->tm_wday;
>> +        MD = (MR_Integer) p->tm_mday;
>> +        YD = (MR_Integer) p->tm_yday;
>> +        N = (MR_Integer) p->tm_isdst;
>> +        ErrorMsg = MR_make_string_const(\"\");
>> +    }
>
> You should be consistent about whether you fill in
> the "success" output args if IsOk = no. Either do it
> in all the foreign procs that return IsOk, or none of them.
> Likewise, set ErrorMsg to "" either in all the IsOk = yes cases,
> or none of them.

Done.

>> -    if (N == 1 & !isDST) {
>> -        // If the time we constructed is not in daylight savings time, but it
>> -        // should be, we need to subtract the DSTSavings.
>> -        java.time.Duration savings = rules.getDaylightSavings(Time0);
>> -        Time = Time0.minus(savings);
>> -        if (!rules.isDaylightSavings(Time)) {
>> -            throw new RuntimeException(
>> -                ""time.mktime: failed to correct for DST"");
>> +    try {
>> +        java.time.ZoneId tz = java.time.ZoneId.systemDefault();
>> +        java.time.Instant Time0 = java.time.ZonedDateTime.of(
>> +            java.time.LocalDateTime.of(Yr + 1900, Mnt + 1, MD, Hrs, Min, Sec),
>> +            tz).toInstant();
>
> Seeing "tz)" at the *start* of a line is jarring.

Do you have an alternative formatting in mind (that doesn't exceed the
80 char line limit)?  Or is your comment related to the use of "tz" as a
local variable name, as opposed to say "time_zone"?

> I am not familiar with the non-C primitives some of this code
> is calling, but as far as I can tell, the rest is fine.

The underlying primitives we call have not changed; we just check
what they're returning more strongly.

Julien.