[m-rev.] for review: improvements to relation.m

[Julien Fischer]

On Wed, 5 Sep 2007, Mark Brown wrote:

> Estimated hours taken: 2
> Branches: main
>
> Improvements to the relation module.
>
> library/relation.m:
> 	Change the type relation_key/0 to relation_key/1.  The argument is
> 	a phantom type which provides some extra type safety.
>
> 	Document this module using more consistent terminology.  In
> 	particular, for relation(T) an "element" refers to a value of
> 	type T, a "key" is of type relation_key(T), and an "edge" is an
> 	ordered pair of keys.
>
> 	s/remove/delete/g, for consistency with other library modules.
>
> 	Export a version of relation.reduced which also returns the map
> 	from relation keys to clique keys.  (This is needed by G12.)
>
> 	Change the stability of this module to "medium", on the grounds
> 	that it's been used reasonably widely now.
>
> library/svrelation.m:
> compiler/dependency_graph.m:
> compiler/hlds_module.m:
> compiler/modules.m:
> compiler/prog_event.m:
> compiler/stratify.m:
> profiler/propagate.m:
> 	Update to make use of the new types.
>

I wonder if this might not be an opportunity to swap the argument
orderings in relation.m around so they match those in svrelation.m.
(We could then delete svrelation.m.)

...

> @@ -612,17 +644,17 @@
>     % Add the arcs to the composition.
>     list.foldl(add_compose_arcs(KeyMap1, KeyMap2), KeyAL, !Compose).
>
> -:- pred find_new_rel_keys(pair(relation_key_set)::in,
> -    relation_key_set::in, relation_key_set::out,
> -    relation_key_set::in, relation_key_set::out) is det.
> +:- pred find_new_rel_keys(pair(relation_key_set(T))::in,
> +    relation_key_set(T)::in, relation_key_set(T)::out,
> +    relation_key_set(T)::in, relation_key_set(T)::out) is det.
>
> find_new_rel_keys(R1Keys - R2Keys,
>         R1NeededKeys0, R1NeededKeys1, R2NeededKeys0, R2NeededKeys1) :-
>     R1NeededKeys1 = sparse_bitset.union(R1NeededKeys0, R1Keys),
>     R2NeededKeys1 = sparse_bitset.union(R2NeededKeys0, R2Keys).
>
> -:- pred add_compose_arcs(key_map::in, key_map::in, pair(relation_key_set)::in,
> -    relation(T)::in, relation(T)::out) is det.
> +:- pred add_compose_arcs(key_map(T)::in, key_map(T)::in,
> +    pair(relation_key_set(T))::in, relation(T)::in, relation(T)::out) is det.
>
> add_compose_arcs(KeyMap1, KeyMap2, R1Keys - R2Keys, !Compose) :-
>     relation.add_cartesian_product(
> @@ -630,8 +662,8 @@
>         map_key_set(KeyMap2, R2Keys),
>         !Compose).

I suggest renaming that so that it refers to edges rather than arcs.

...

>     % Provided that we never encounter a node that we've visited before
> @@ -738,8 +770,8 @@
>     relation.components_2(Rel, DomList, set.init, SetofBitsets),
>     Set = set.map(to_set, SetofBitsets).
>
> -:- pred relation.components_2(relation(T)::in, list(relation_key)::in,
> -    set(relation_key_set)::in, set(relation_key_set)::out) is det.
> +:- pred relation.components_2(relation(T)::in, list(relation_key(T))::in,
> +    set(relation_key_set(T))::in, set(relation_key_set(T))::out) is det.
>
> relation.components_2(_Rel, [], !Comp).
> relation.components_2(Rel, [X | Xs], !Comp) :-
> @@ -747,13 +779,13 @@
>     queue.list_to_queue([X], Q0),
>     relation.reachable_from(Rel, Q0, Set0, Component),
>     set.insert(!.Comp, Component, !:Comp),
> -    list_to_set(Xs, XsSet `with_type` relation_key_set),
> +    list_to_set(Xs, XsSet `with_type` relation_key_set(T)),

You may as well replace `with_type` with `:` there.

The rest of that looks fine.

Julien.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------