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

[Julien Fischer]

On Mon, 6 Jul 2026 at 17:42, Zoltan Somogyi <zoltan.somogyi at runbox.com> wrote:
>
>
> 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.

Done.

> > +:- 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).

I went with read_int_and_return_num_digits

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

Renamed to NumDigits.  Val is renamed to Int to match the code further below.

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

Renamed to read_int_and_return_num_digits_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.

Done.

> In later predicates, I think it should be all separate
> named variables.

Actually, the non-state variables can similarly be eliminated in
the later predicates.  (And in a few other parts of this module
not included in this version of the diff.)

> > +    % 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/

Done.

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

Done.

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

Done.

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

The non-state variables are unnecessary here; I have simplified the above
accordingly.

> Apart from the above, the diff is fine.

Thanks for that.

Julien.