[m-rev.] Constraint-based typechecking

[Zoltan Somogyi]

On 18-Feb-2009, Ralph Becket <rafe at csse.unimelb.edu.au> wrote:
> Good work!

Seconded. Thanks, Andrew.

What follows is a review of Rafe's review, not a review of Andrew's diff,
since I haven't had time yet for the latter.

> > +    % A set of types. tdomain_any represents the set of all types, for
> > unbound
> 
> The convention is to put two spaces after a full stop.

For some people. Others, like me, put only one.

> > +
> > +update_goal(PredEnv, ConstraintMap, !Goal, Errors) :-
> > +    Disjunctions = map.values(ConstraintMap),
> > +    list.filter_map(has_one_disjunct, Disjunctions, Conjunctions),
> > +    list.filter_map(pred_constraint_info, Conjunctions, PredDataA),
> > +    list.filter_map(has_multiple_disjuncts, Disjunctions, AmbigDisjuncts),
> > +    list.filter_map(diagnose_ambig_pred_error(PredEnv), AmbigDisjuncts,
> > +        Errors),
> > +
> > +    list.map(list.filter_map(pred_constraint_info), AmbigDisjuncts,
> > +        AmbigPredData),
> > +    PredDataB = list.filter_map(list.head, AmbigPredData),
> > +    list.append(PredDataA, PredDataB, PredData),
> 
> Use PredData = PredDataA ++ PredDataB instead.

You should also find better names than PredDataA and B.

> > +equal_domain(tdomain(A), tdomain(B)) :-
> > +    (
> > +        set.count(A, C),
> > +        set.count(B, C)
> 
> Why not write set.count(A) = set.count(B) instead?

Because that causes a type ambiguity; the compiler doesn't know whether
you are comparing integers or partial predicate applications.

> > +has_singleton_domain(DomainMap, TVar) :-
> > +    map.search(DomainMap, TVar, tdomain(Domain)),
> > +    set.singleton_set(Domain, _).
> 
> It's more efficient to write set.count(Domain) = 1 here since
> set.singleton_set/2 will allocate some memory.

No, Andrew's code is more efficient. It won't allocate memory in the
(in, out) grade, and it takes constant time, whereas set.count takes
linear time.

> > +    list(type_constraint_set)::in, list(type_constraint_set)::out) is det.
> > +
> > +next_min_unsat(Constraint, C, !D, !P, !MinUnsats) :-
> > +    svset.delete(C, !P),
> > +    min_unsat_constraints(Constraint, !.D, !.P, !MinUnsats),
> > +    svset.insert(C, !D).
> > +
> > +:- pred add_message_to_spec(error_msg::in, type_constraint_info::in,
> > +    type_constraint_info::out) is det.
> > +
> > +add_message_to_spec(ErrMsg, !TCInfo) :-
> > +    Spec = error_spec(severity_error, phase_type_check, [ErrMsg]),
> > +    !:TCInfo = !.TCInfo^tconstr_error_spec :=
> > +        [Spec | !.TCInfo^tconstr_error_spec ].

That should be simplified to

	!TCInfo := [Spec | !.TCInfo^tconstr_error_spec].

I agree with all of Rafe's other comments.

Zoltan.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------