[m-rev.] problems with abs for int{8,16,32}

Zoltan Somogyi zoltan.somogyi at runbox.com
Fri Feb 16 20:32:18 AEDT 2018

On Fri, 16 Feb 2018 03:47:20 -0500 (EST), Julien Fischer <jfischer at opturion.com> wrote:
> If you have not fixed this or are not intending to fix it anytime soon,
> could you please let me know and I will have a look at it this weekend.

I just committed two diffs cleaning up some aspects of unify_gen.m,
and I intended to commit a third before fixing the problem.

I am pretty sure the bug lies in the fact that generate_const_struct
ignores the cs_term_type field of ConstStruct. It should look up the
representation of the type, and use that info in its task.

The current code of generate_cons_struct_arg_tag handles each arg
independently; this needs to be fixed to allow it to combine the values
of 2+ args that happen to be packed into the same word. The code
that does that can then set the "typed" part of the typed-rval
it generates to a word-sized type (I expect lt_integer for such packed words).
As it is, the current code of generate_cons_struct_arg_tag almost ignores ArgWidth;
using the natural type of the int8 argument for the type part of the typed-rval
is what causes the wrong type for the static structure generated for [-128i8].

In ml_unify_gen.m, the code that constructs static terms is modelled
after the code that constructs dynamic terms, with (IIRC) close to the
minimum changes required. This should also be the case for unify_gen.m,
but I wouldn't want to take the code generating dynamic terms as the model
for the static case just yet. This is because generate_cons_args *also*
ignores argument packing, and requires a post-pass (pack_cell_rvals)
to fix this. I would prefer to replace the generate_cons_args/pack_cell_rvals
two-pass structure with a single pass structure in the dynamic case
before using it as the model for the static case. This is the third
cleanup I mentioned above.

I won't get to either the third cleanup or the bug fix before tomorrow morning
at the earliest, and I intend to go shopping then, but I was expecting
to finish both by the end of sunday. Is that soon enough for you,
or do you want to take over from here?


More information about the reviews mailing list