[m-rev.] for review: bulk unify and compare

Zoltan Somogyi zoltan.somogyi at runbox.com
Tue Oct 2 13:33:16 AEST 2018



On Tue, 2 Oct 2018 11:35:19 +1000, Peter Wang <novalazy at gmail.com> wrote:
> > -    % generate_linear_compare_cases: for a type such as
> > +    % generate_linear_compare_cases does a part of the job assigned to
> > +    % generate_compare_proc_body_du_linear. Specifically, for a type such as
> 
> s/generate_linear_compare_cases/generate_compare_du_linear_cases
> (earlier in the file as well)

Done.

> >      %
> >      %   :- type foo
> >      %       --->    f
> >      %       ;       g(a)
> >      %       ;       h(b, foo).
> >      %
> > -    % we want to generate code
> > +    % we generate
> >      %
> >      %   (
> >      %       X = f,      % UnifyX_Goal
> > -    %       Y = X,      % UnifyY_Goal
> >      %       R = (=)     % CompareArgs_Goal
> >      %   ;
> >      %       X = g(X1),
> > @@ -1242,21 +1513,19 @@ generate_compare_proc_body_du_linear(SpecDefnInfo, Ctors, Res, X, Y, Goal,
> >      %       )
> >      %   )
> >      %
> > -    % Note that in the clauses for constants, we unify Y with X, not with
> > -    % the constant. This is to allow dupelim to eliminate all but one of
> > -    % the code fragments implementing such switch arms.
> > -    %
> >  :- pred generate_compare_du_linear_cases(spec_pred_defn_info::in,
> 
> This did confuse me until I found the context for it in the comment
> about generate_compare_proc_body_du_linear. Not sure if anything needs
> to be done.

What is the "this" that confused you?

> > +:- type maybe_all_args_in_word_so_far
> > +    --->    not_all_args_in_word_so_far
> > +    ;       all_args_in_word_so_far.
> 
> > +:- type maybe_packed_word_ops
> > +    --->    no_packed_word_ops
> > +    ;       some_packed_word_ops.
> 
> Can you add comments or rename the constructors in both types
> to make their meaning more obvious? e.g.
> 
>     :- type unify_proc_maybe_packed_word_ops
> 	--->	unify_proc_uses_packed_word_ops
> 	;	unify_proc_does_not_use_packed_word_ops.

I try to keep the type name short enough to allow a state var argument pair
to be declared on one line :-(

How about used_no_packed_word_ops/used_some_packed_word_ops?

I added a comment for the other type.

Zoltan.


More information about the reviews mailing list