[m-rev.] for review: allow optional underscores in numeric literals

[Paul Bone]

On Wed, Jan 11, 2017 at 02:07:55PM +1100, Julien Fischer wrote:
> 
> Hi Paul,
> 
> On Wed, 11 Jan 2017, Paul Bone wrote:
> 
> >On Mon, Jan 09, 2017 at 04:38:24PM +1100, Julien Fischer wrote:
> >>Allow the optional use of underscores in numeric literals for the purpose of
> >>improving their readability (e.g. by grouping digits etc).  We allow any number
> >>of underscores between digits and also between the radix prefix (if present) and
> >>the initial digit.  (When integer type suffixes are supported we will also
> >>allow them to be preceded by any number of underscores.)  The following are
> >>*not* allowed:
> >>
> >>   1. Leading underscores.
> >>   2. Trailing underscores.
> >>   3. Underscores inside the components of a radix prefix (e.g.
> >>      0_xffff or 0__b101010.)
> >>   4. Underscores immediately adjacent to the decimal point in a float
> >>      literal (e.g. 123_._123.)
> >>   5. Underscores immediately adjacent to the exponent ('e' or 'E) in
> >>      a float literal (e.g. 123_e12 or 123E_12.)
> >>   6. Underscores immediately adjacent to the optional sign of an exponent
> >>      in a float literal (e.g. 123_+e12 or 123-_E12.)
> >>   7. Underscores between the optional sign of an exponent and the exponent
> >>      indicator (e.g. 123+_e12.)
> >
> >These make sense.  There's no point in supporting them as they don't improve
> >readability or group digits.
> >
> >Does this leave only underscores within sequences of digits or are there
> >other cases that we might also want to exclude?
> 
> It leaves (1) underscores within sequences of digits, (2) between a
> radix prefix and sequence of digits and eventally (3) between the
> last digit and a suffix.
> 
> I think (2) and (3) are worth having where, for example, you have
> complicated groupings of bits within binary literals.

I see no problem with keeping these.  Specifically I can't imagine they'd
make the implementation more complex.

> >>diff --git a/library/lexer.m b/library/lexer.m
> >>index c527ffd..fddd8aa 100644
> >>--- a/library/lexer.m
> >>+++ b/library/lexer.m
> >>@@ -1912,13 +1976,19 @@ get_zero(Stream, Token, !IO) :-
> >>         )
> >>     ).
> >>
> >>+:- type last_digit_is_underscore
> >>+    --->    last_digit_is_underscore
> >>+    ;       last_digit_is_not_underscore.
> >>+
> >
> >Could you add a comment explaining the purpose of this type?  I
> >mistakeningly thought that the above code was saying what the last digit of
> >the entire token was, rather than saying what the last digit _that it had
> >seen_ was.  Thanks.
> 
> Ok, how's this:
> 
>      This type records whether the last digit seen by the lexer as it
>      processes a numeric token was an underscore or not.
>      Note that there may be other intervening characters in the token
>      between the last digit and the current one (e.g. the decimal point
>      in a float literal.)

The only thing I'd add is "...so that invalid literals can be rejected" or
similar.  No need to re-send this for review.

> >>diff --git a/library/string.m b/library/string.m
> >>index 1c4299a..cb70509 100644
> >>--- a/library/string.m
> >>+++ b/library/string.m
> >>@@ -5476,6 +5486,97 @@ accumulate_negative_int(Base, Char, N0, N) :-
> >>
> >> %---------------------%
> >>
> >>+base_string_to_int_underscore(Base, String, Int) :-
> >>+    index(String, 0, Char),
> >>+    End = count_code_units(String),
> >>+    ( if Char = ('-') then
> >>+        End > 1,
> >>+        foldl_between(base_negative_accumulator_underscore(Base), String, 1, End, 0, Int)
> >
> >Why create extra code rather than doing as you did above and parse the
> >number as a positive number then multiply by -1?
> 
> (1) I just cut-and-pasted-and-modfied the existing code for
> base_string_to_int/3 and (2) you need to fail on overflow and the test
> for that is different in the negative case.

Ah.

> >>*.exp
> >
> >I would prefer that we call these literals and update other docs or messages
> >either now or later.
> 
> I'm fine with "literals" as well.  I will update all the error messages
> as part of the change that provides the user facing documentation.
> 
> >Otherwise this change is good.  Thanks.
> 
> Thanks for the review.

np.

-- 
Paul Bone
http://paul.bone.id.au