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

[Zoltan Somogyi]

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

>   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.

> @@ -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.

> -    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.

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.

Zoltan.