[m-rev.] for review: tightening mlds_type

[Zoltan Somogyi]

On Thu, 27 Sep 2018 12:04:22 +0000 (UTC), Julien Fischer <jfischer at opturion.com> wrote:

> 
> Hi Zoltan,
> 
> On Thu, 27 Sep 2018, Zoltan Somogyi wrote:
> 
> > We discussed this a few weeks ago. For review by anyone.
> 
> > Tigthen the mlds_type type.
> 
> s/Tigthen/Tighten/

Fixed.

> > @@ -822,22 +820,39 @@ make_dummy_first_soln_row(FirstSolnStructType, FieldTypes,
> >
> >  ml_default_value_for_type(MLDS_Type) = DefaultRval :-
> >      (
> > -        MLDS_Type = mlds_native_int_type,
> > -        DefaultRval = ml_const(mlconst_int(0))
> > +        MLDS_Type = mlds_builtin_type_int(IntType),
> > +        (
> > +            ( IntType = int_type_int
> > +            ; IntType = int_type_int8
> > +            ; IntType = int_type_int16
> > +            ; IntType = int_type_int32
> > +            ; IntType = int_type_int64
> > +            ; IntType = int_type_uint8
> > +            ; IntType = int_type_uint16
> > +            ; IntType = int_type_uint32
> > +            ; IntType = int_type_uint64
> > +            ),
> > +            % XXX This is wrong in signedness and/or size
> > +            % for all of the above except int_type_int.
> > +            DefaultRval = ml_const(mlconst_int(0))
> 
> Is there a reason not to use the correct values now?

The code above preserved existing behavior, but you are right,
there is no reason. I have updated the code to generate zeros
with the named size and signedness.

> > diff --git a/compiler/options.m b/compiler/options.m
> > index 03aed55..6768e6f 100644
> > --- a/compiler/options.m
> > +++ b/compiler/options.m
> > @@ -1456,11 +1456,11 @@ option_defaults_2(compilation_model_option, [
> >                                          % all word bits for argument packing.
> >      allow_double_word_fields            -   bool(yes),
> >      allow_double_word_ints              -   bool(no),
> > -    allow_packing_dummies               -   bool(no),
> > -    allow_packing_ints                  -   bool(no),
> > -    allow_packing_chars                 -   bool(no),
> > -    allow_packing_local_sectags         -   bool(no),
> > -    allow_packing_remote_sectags        -   bool(no),
> > +    allow_packing_dummies               -   bool(yes),
> > +    allow_packing_ints                  -   bool(yes),
> > +    allow_packing_chars                 -   bool(yes),
> > +    allow_packing_local_sectags         -   bool(yes),
> > +    allow_packing_remote_sectags        -   bool(yes),
> >      allow_packing_mini_types            -   bool(no),
> >      sync_term_size                      -   int(8),
> >                                          % 8 is the size on linux (at the time
> 
> Did you mean to enable these now?

Not quite yet, but soon. Thanks for spotting it.

Zoltan.