[m-rev.] for review: changes to the library from the mode-constraints branch

[Fergus Henderson]

On 19-Aug-2002, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Move changes in the library on the mode-constraints branch onto the trunk.
> 
> library/robdd.m:
> 	Add this module, which provides a Mercury interface to the C code in
> 	robdd/bryant.c. In some places, robustness has been sacrificed for
> 	speed, and the module is not (yet) as well documented as it could be;
> 	therefore it is not (yet) included in the documentation.

Hmm... so for users, the only noticable effect will be an increase in
the object code size of the library?
That doesn't seem like an improvement.

If it's not yet ready for prime-time, what's the advantage of putting
it in the library?

> library/pprint.m:
> 	Print robdds nicely, since this is essential to debugging code handling
> 	robdds. (This is why adding robdd.m in some other directory, e.g. the
> 	compiler, wouldn't really work.)

Oh.  I see.  :-(

I guess it's OK to add it to the library then.

The problem here is that pprint is not very extensible.
I wish we had dynamic type class casts...

> Index: library/robdd.m

There should be some XXX comments in this file explaining why it is not
yet ready for inclusion in the library reference manual.

> +:- implementation.
> +:- pragma foreign_decl("C", "
> +
> +#define	NDEBUG
> +#define	CLEAR_CACHES
> +#define	COMPUTED_TABLE
> +#define	EQUAL_TEST
> +#define	USE_ITE_CONSTANT
> +#define	NEW
> +#define	RESTRICT_SET
> +
> +#include ""../robdd/bryant.h""
> +").

This is not namespace-clean.

With intermodule optimization, won't this leak out into
C code in Merury modules which import the "library" module
but don't use the "robdd" module at all?

If so, then I don't think we should add robdd to the library.

> +#include ""../robdd/bryant.c""

This one is also not namespace-clean.
bryant.c defines frequently occuring names such "copy"
which may lead to link errors when linking in other
object files which use the same name.

We should not include robdd in the library unless/until this is fixed, IMHO.

> +:- pragma foreign_proc("C",
> +	one = (F::out),
> +	[will_not_call_mercury, promise_pure],
> +"
> +	F = (MR_Word) trueVar();
> +").

Does this compile with --high-level-data (grade hl.gc)?

It's OK if there are warnings, but please test that it still builds
in grade hl.gc.

> Index: library/sparse_bitset.m
> @@ -168,6 +174,20 @@
>  :- pred remove_list(sparse_bitset(T), list(T), sparse_bitset(T)) <= enum(T).
>  :- mode remove_list(in, in, out) is semidet.
>  
> +	% `remove_leq(Set, X)' returns the list Set with all elements less than
> +	% or equal to X removed.
> +:- func remove_leq(sparse_bitset(T), T) = sparse_bitset(T) <= enum(T).

"the list Set"?  It's not a list.

I suggest:

	% `remove_leq(Set, X)' returns `Set' with all the elements less
	% than or equal to `X' removed.  In other words, it returns
	% the set containing all the elements of `Set' which are greater
	% than `X'.

Likewise for remove_gt.

> +	% `filter(Pred, Set)' removes element from Set for which Pred fails.

I suggest adding

	% In other words, it returns the set consisting of those
	% elements of `Set' for which `Pred' succeeds.

> +++ library/term.m	2002/08/19 03:47:05
> +:- func term__var_supply_max_var(var_supply(T)) = var(T).
> +% 	term__var_supply_max_var(VarSupply):
> +%		returns the highest numbered variable returned from this
> +%		var_supply.

What does "highest numbered" mean?

Is that the maximum in the ordering specified by compare/3,
or the highest value returned by term__var_to_int?

What's the purpose of providing this interface?

> +:- func map__max_key(map(K, V)) = K is semidet.
> +
> +:- func map__min_key(map(K, V)) = K is semidet.

These should be documented.

> Index: library/varset.m
> +:- func varset__max_var(varset(T)) = var(T).

This should be documented.
My comments for var_supply_max_var above also apply here.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------