[m-rev.] for review: fix a bug in duration parsing

[Zoltan Somogyi]

On Mon, 6 Jul 2026 16:53:50 +1000, Julien Fischer <jfischer at opturion.com> wrote:
> +:- pred read_char(char::out, list(char)::in, list(char)::out) is semidet.
> +
> +read_char(Char, [Char | Rest], Rest).

I would call this pred read_NEXT_char, because you later have
predicates that look for just a SPECIFIC char.

> +:- pred read_int_and_num_chars(int::out, int::out,
> +    list(char)::in, list(char)::out) is det.

This does not read num chars beyond reading ints.
I would rename it to read_int_and_return_{num_char,len}.

> +read_int_and_num_chars(Val, N, !Chars) :-
> +    read_int_and_num_chars_2(0, Val, 0, N, !Chars).

N should be something like Len, because Val and N are thoroughly
confusable. Or NumDigits, which you use later.

> +:- pred read_int_and_num_chars_2(int::in, int::out, int::in, int::out,
> +    list(char)::in, list(char)::out) is det.

This pred name should be something like read_int_and_return_len_loop.

> +read_int_and_num_chars_2(!Val, !N, !Chars) :-
> +    ( if
> +        !.Chars = [Char | Rest],
> +        decimal_digit_to_int(Char, Digit)
> +    then
> +        !:Val = !.Val * 10 + Digit,
> +        read_int_and_num_chars_2(!Val, !.N + 1, !:N, Rest, !:Chars)
> +    else
> +        true
> +    ).

I think that having both state variables and non-state variables
representing the different parts of a list (or any data structure)
is a recipe for confusion. Here, I think Rest could be just the next
value of !.Chars. In later predicates, I think it should be all separate
named variables.

> +    % Read an optional leading minus sign. Sign is -1 if one is present (and is
> +    % consumed), and 1 otherwise (consuming nothing).
> +    %
>  :- pred read_sign(int::out, list(char)::in, list(char)::out) is det.

s/Sign is/Set Sign to/

>  read_days(Days, !Chars) :-
>      read_int_and_char_or_zero(Days, 'D', !Chars).

I would flip the order of the first two args. 'D' is input; Days is output.
I think that is more important than following the order in the date string,
which is in the name anyway.

Also, I think a name such as read_int_and_GIVEN_char_or_RETURN_zero
would be clearer.

> -:- pred read_hours(int::out, list(char)::in, list(char)::out) is det.
> +:- pred read_hours(int::out, list(char)::in, list(char)::out) is semidet.
> 
>  read_hours(Hours, !Chars) :-
>      read_int_and_char_or_zero(Hours, 'H', !Chars).
> 
> -:- pred read_minutes(int::out, list(char)::in, list(char)::out) is det.
> +:- pred read_minutes(int::out, list(char)::in, list(char)::out) is semidet.
> 
>  read_minutes(Minutes, !Chars) :-
>      read_int_and_char_or_zero(Minutes, 'M', !Chars).
> 
> +    % read_int_and_char_or_zero(Int, Char, !Chars):
> +    %
> +    % Read one optional "<integer><Char>" duration component from the front of
> +    % !.Chars, where <integer> is a run of decimal digits.
> +    % If !.Chars begins with Char preceded by at least one digit, then set Int
> +    % to the value of those digits and !:Chars to the characters after Char.
> +    % If !.Chars begins with Char with no digits before it, then fail.
> +    % Otherwise, set Int to zero and leave !:Chars unchanged.
> +    %
> +:- pred read_int_and_char_or_zero(int::out, char::in,
> +    list(char)::in, list(char)::out) is semidet.
> +
> +read_int_and_char_or_zero(Int, Char, !Chars) :-
> +    ( if
> +        read_int_and_num_chars(Int0, NumDigits, !.Chars, Chars1),
> +        Chars1 = [Char | Rest]
> +    then
> +        NumDigits > 0,
> +        !:Chars = Rest,
> +        Int = Int0
> +    else
> +        Int = 0
> +    ).

This is an example where I think the state var !Chars hurts more than helps.

Apart from the above, the diff is fine.

Zoltan.