[m-rev.] for review: extend the lexer to recognise additional integer literals

[Julien Fischer]

Hi Zoltan,

On Wed, 26 Apr 2017, Zoltan Somogyi wrote:

> On Tue, 25 Apr 2017 21:39:33 +1000 (AEST), "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
>
>>
>>
>> On Tue, 25 Apr 2017 21:10:14 +1000 (AEST), Julien Fischer <jfischer at opturion.com> wrote:
>>> For review by anyone.
>>
>> I will review this later today.
>
> Here is the review.
>
>> --- a/library/lexer.m
>> +++ b/library/lexer.m
>> ...
>> @@ -1969,7 +1969,7 @@ get_zero(Stream, Token, !IO) :-
>>             get_number(Stream, LastDigit, [Char], Token, !IO)
>>         else if Char = '_' then
>>             LastDigit = last_digit_is_underscore,
>> -            get_number(Stream, LastDigit, [], Token, !IO)
>> +            get_number(Stream, LastDigit, ['0'], Token, !IO)
>>         else if Char = '''' then
>>             get_char_code(Stream, Token, !IO)
>>         else if Char = 'b' then
>
> Document why we pass ['0'] instead of []: get_zero is invoked
> only if the last (non-underscore) char seen was a zero.

Done.

> Maybe this would be easier to do if get_zero took a !.RevChars
> argument, which already contained ['0'].

Maybe; I'm not sure it particuarly matters.

>
>> @@ -1978,6 +1978,12 @@ get_zero(Stream, Token, !IO) :-
>>             get_octal(Stream, Token, !IO)
>>         else if Char = 'x' then
>>             get_hex(Stream, Token, !IO)
>> +        else if Char = 'u' then
>> +            get_integer_size_suffix(Stream, ['0'], base_10, unsigned,
>> +                Token, !IO)
>> +        else if Char = 'i' then
>> +            get_integer_size_suffix(Stream, ['0'], base_10, signed,
>> +                Token, !IO)
>
> Likewise: document why we pass ['0'] instead of [] in these places.

Done.

>> @@ -2156,6 +2179,7 @@ get_binary_2(Stream, !.LastDigit, !.RevChars, Token, !IO) :-
>>     posn::in, posn::out) is det.
>>
>> string_get_binary_2(String, !.LastDigit, Len, Posn1, Token, Context, !Posn) :-
>> +    Posn2 = !.Posn,
>>     ( if string_read_char(String, Len, Char, !Posn) then
>>         ( if char.is_binary_digit(Char) then
>>             !:LastDigit = last_digit_is_not_underscore,
>> @@ -2165,12 +2189,21 @@ string_get_binary_2(String, !.LastDigit, Len, Posn1, Token, Context, !Posn
> ) :-
>>             !:LastDigit = last_digit_is_underscore,
>>             string_get_binary_2(String, !.LastDigit, Len, Posn1, Token,
>>                 Context, !Posn)
>> +        else if Char = 'u' then
>> +            string_get_integer_size_suffix(String, Len, Posn1, Posn2, base_2,
>> +                unsigned, Token, !Posn),
>> +            string_get_context(Posn1, Context, !Posn)
>> +        else if Char = 'i' then
>> +            string_get_integer_size_suffix(String, Len, Posn1, Posn2, base_2,
>> +                signed, Token, !Posn),
>> +            string_get_context(Posn1, Context, !Posn)
>
> With Posn1, Posn2, !.Posn and !:Posn, I lost track.
> A comment or two might help, and so might a more expressive name
> than "Posn2". (Not just here; in all the places that follow this
> code schema.)

I have renamed Posn2 to LastDigitPosn and added an explanatory comment.
Much of this module uses Posn1 (and indeed Posn0), which isn't ideal.
I will look into renaming them as well -- indeed, I would have done so
only I'm not sure that the usage of Posn1, in particular, is uniform
across the module.

> I am too tired now to review the diff of the test cases with sufficient
> attention; I will do that tomorrow. However, since the rest of the diff
> is fine, you can go ahead and commit it.

Ok, thanks for the review.

Julien.