[m-rev.] for review: rtti_varmaps

[Mark Brown]

On 18-Jul-2005, Julien Fischer <juliensf at cs.mu.OZ.AU> wrote:
> On Sun, 17 Jul 2005, Mark Brown wrote:
> > 	Define the rtti_varmaps abstract data type.  This data structure
> > 	consists of the type_info_varmap and the typeclass_info_varmap.
> > 	A new map, type_info_type_map, is added,  which is like the inverse
> 
> That's a little awkward.  How about:
> 
> 	Add a new map, type_info_var_map, that is ...

Done.

> > 	The predicates rtti_det_insert_type_info_locn and set_type_info_locn,
> > 	which update the type_info_varmap, contain sanity checks to ensure
> > 	that only type variables that have already been registered with
> > 	the type_info_type_map are used, and that the information in both
> > 	maps are consistent.
> >
> s/are/is/

Done.

> > library/injection.m:
> > 	New library module.  This provides an `injection' type which is
> > 	similar to the existing `bimap' type in that it implements
> > 	back-to-back maps, but doesn't have such stringent invariants
> > 	imposed.  In particular, the reverse map is not required to be
> > 	innjective.
> s/innjective/injective/
> 

Done.

> > +		% Traverse TVarsList again, this time looking for locations
> > +		% in later branches that merge with locations in the first
> > +		% branch.  When we find one, add a type substitution which
> > +		% represents the type variables that thus got merged.
> s/thus got/were/
> 

Done.

> >  %-----------------------------------------------------------------------------%
> >
> > +	% Types and predicates to store information about RTTI.
> > +
> 
> If this is a section heading, it should be formatted as the coding
> standard suggests. e.g.
> 
> 	%-------------------------------------...
> 	%
> 	% Types and predicates to store information about RTTI
> 	%

Done.

> > +:- pred maybe_check_type_info_var(type_info_locn::in, tvar::in,
> > +	rtti_varmaps::in, rtti_varmaps::out) is det.
> > +
> > +maybe_check_type_info_var(type_info(Var), TVar, !VarMaps) :-
> > +	(
> > +		map__search(!.VarMaps ^ ti_type_map, Var, Type)
> > +	->
> > +		(
> > +			Type = term__variable(TVar)
> > +		->
> > +			true
> > +		;
> > +			error("maybe_check_type_info_var: inconsistent info")
> Code inside the compiler should ideally call
> error_util.unexpected/2 rather than error/1.

Done, to all the places where my change calls error/1.  There are still
plenty of other places in the module where error/1 is called, though, but
that can be fixed later.

> > +% The invariants on this data structure, which are enforced by this module,
> > +% are as follows:
> > +%
> 
> There is a space-time trade-off involved inmplementing efficient reverse
> lookups that I feel ought to mentioned here (it should probably also be mentioned
> in bimap's documentation as well).

Done, and I've made a similar change to bimap.m.  The new module doesn't
just provide a different space-time tradeoff, though; it also adds new
functionality not provided by maps alone (or bimaps), namely that you can
do reverse lookups on values that are not actually associated with any key
in the forward mapping.  I've added a comment to make this clearer.

> > +	% Initialize an empty injection.
> > +	%
> > +:- func injection.init = injection(K, V).
> > +:- pred injection.init(injection(K, V)::out) is det.
> > +
> Is the predicate version of injection.init really necessary?

Well, no.  But since it is a library module I've added both so that users
of the module can keep things consistent with how they currently write code.
(For this reason I think that all of the library modules should continue
to provide both function and predicate versions where reasonable.)

> > +	% Return the list of all keys in the injection.
> > +	%
> > +:- func injection.keys(injection(K, V)) = list(K).
> > +:- pred injection.keys(injection(K, V)::in, list(K)::out) is det.
> > +
> I'd argue for not including the predicate versions of procedures,
> like injection.keys, that would naturally be functions; with the
> exception of procedures that are candidate for use with
> state variable notation.

For the above reason I think we should provide both.  It's also consistent
with what's in map.m and bimap.m.

> > +	% Succeeds if the injection contains the given key.
> > +	%
> > +:- pred injection.contains_key(injection(K, V), K).
> > +:- mode injection.contains_key(in, in) is semidet.
> > +
> Is there any reason you haven't used predmode syntax here (and below)?

No.  Fixed.

> A lot of the code in the implementation of this module would benefit
> from using state variable notation, if only to reduce the number of
> F0s, R0s etc.

Done.

> 
> The rest of the diff look okay; assuming that it bootraps in several
> different grades (one of which should definitely be a term size
> profiling grade), you can commit it.

Yep.  Bootstraps in several grades including a tsw grade.

Cheers,
Mark.

--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------