[m-dev.] for review: compressing RTTI using non-word creates (part 2)

[Zoltan Somogyi]

>> +% The field containing the number of live data items is encoded by the
>> +% formula (#Long << 8 + #Short), where #Short is the number data items whose
>> +% descriptions fit into an MR_Short_Lval and #Long is the number of data
>> +% items whose descriptions do not.
> 
> What happens if #Short > 255?
> I think you should detect that case and handle it appropriately.

The number of distinct integers that fit into 8 bits must also fit into 8 bits.

> Why `byte_bits'?  Is there any reason why representing things as e.g.
> "#Long << 10 + #Short" wouldn't work?  If not, then the named constant
> that names this hard-coded shift count should be named something other
> than `byte_bits'. 

As shown the above, this suggestion is built on an incorrect assumption.
The size of the integers in question is designed specifically to be a byte
(actually, uint_least8_t), so byte_bits as a name is perfectly reasonable.

> > -     for (i = 0; i < label->MR_sll_var_count; i++) {
> > +     for (i = 0; i < MR_all_desc_var_count(vars); i++) {
> >               if (streq(vars->MR_slvs_names[i], name)) {
> > -                     return MR_lookup_live_lval_base(
> > -                             vars->MR_slvs_pairs[i].MR_slv_locn, saved_regs,
> > -                             MR_saved_sp(saved_regs),
> > +                     if (i < MR_long_desc_var_count(vars)) {
> > +                             return MR_lookup_long_lval_base(
> > +                                     MR_long_desc_var_locn(vars, i),
> > +                                     saved_regs, MR_saved_sp(saved_regs),
> > +                                     MR_saved_curfr(saved_regs), succeeded);
> > +                     } else {
> > +                             return MR_lookup_short_lval_base(
> > +                                     MR_short_desc_var_locn(vars, i),
> > +                                     saved_regs, MR_saved_sp(saved_regs),
> >                               	  MR_saved_curfr(saved_regs), succeeded);
> > +                     }
> >               }
> >       }
> 
> I think it would be better to avoid duplicating the common code here,
> and write that as
> 
>                         Word locn;      /* or whatever type is appropriate */
>                         if (i < MR_long_desc_var_count(vars)) {
>                                 locn = MR_long_desc_var_locn(vars, i);
>                         } else {
>                                 locn = MR_short_desc_var_locn(vars, i);
>                         };
>                         return MR_lookup_live_lval_base(locn, saved_regs,
>                                 MR_saved_sp(saved_regs),
>                                 MR_saved_curfr(saved_regs), succeeded);

The code you factored out wasn't common; one handles long location descriptions
and one handles short ones. They are different, although many of their
arguments are the same. There is no such function as MR_lookup_live_lval_base,
and there is no meaningful way to create one that does not reduce to the
scheme I have above.

Zoltan.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------