[m-dev.] cvs diff - library additions

[Fergus Henderson]

Christopher Rodd SPEIRS, you wrote:
> 
> I have made a number of additions to the library.  The main changes are in
> bag.m.  These changes to bag.m required some changes to be made to
> code_exprn.m.  The rest of the changes are additions to list.m, io.m and
> map.m.
>         The additions were mainly where there seemed to be 'missing'
> predicates.  For example, there was list__merge/3,
> list__merge_and_remove_dups/3, list__merge/4, but no
> list__merge_and_remove_dups/4.
> 
>         The changes to io.m are: I added io__tmpnam, and io__unlink_file,
> which simply call the respective c functions.
> 
> Would someone please review these changes...

OK, I will.

Adding stuff to the standard library is not something one
should do lightly -- each addition has a definite cost (increased
size of the library documentation, increased size of the library
archive/shared object files that we need to distribute), and
once something is added it is quite difficult to remove it.
I think most of your changes are probably worthwhile, but we
need to consider each one carefully.

Any changes to the library should be documented briefly in the NEWS file.
(This is particularly important for changes that break backwards
compatibility, as yours do.)

With regard to your changes to io.m, it might be better to work on
provide a full Posix interface (the Perl Posix support is probably a
good thing to look at) rather than adding each of the functions
one-at-a-time as we need them.

My detailed comments follow...

--------------------

Diffs should come with their CVS log message.
Your explanatory text above is not in the standard log message template format.

> +	% delete one occurrence of a particular value from a bag
> +:- pred bag__delete(bag(T), T, bag(T)).
> +:- mode bag__delete(in, in, out) is det.

What happens if the object isn't in the bag?

> +	% bag__subtract(Bag0, SubBag, Bag)
> +	% subtracts SubBag from Bag0 to produce Bag
> +:- pred bag__subtract(bag(T), bag(T), bag(T)).
> +:- mode bag__subtract(in, in, out) is det.

It's not at all clear what the semantics of subtraction for bags are.

> +	% The third bag is the union of the first 2 bags.
> +	% {1, 1, 2, 2} U {2, 2, 3, 3} = {1, 1, 2, 2, 2, 2, 3, 3}

Add "e.g." at the start of that line.

> +	% The third bag is the intersection of the first 2 bags
> +:- pred bag__intersect(bag(T), bag(T), bag(T)).
> +:- mode bag__intersect(in, in, out) is det.

It's not clear what the semantics of intersection for bags are.

> +	% fails if there is no intersection between the 2 bags
> +	% bag__intersect(A, B) :- bag__intersect(A, B, C), bag__is_empty(C).
> +:- pred bag__intersect(bag(T), bag(T)).
> +:- mode bag__intersect(in, in) is semidet.

Shouldn't that be
	% bag__intersect(A, B) :- bag__intersect(A, B, C), not bag__is_empty(C).
							   ^^^

Is there any particular reason for having this predicate,
rather than just writing `bag__intersect(A, B, C), not bag__is_empty(C)'?

> +	% fails if the first bag is not a subbag of the second.
> +:- pred bag__is_subbag(bag(T), bag(T)).
> +:- mode bag__is_subbag(in, in) is semidet.

It's not clear what the semantics of intersection for bags are.

> +bag__det_remove(Bag0, Item, Bag) :-	% det
> +	( bag__remove(Bag0, Item, Bag1) ->
> +		Bag = Bag1
> +	;
> +		error("Missing Item in bag."),

Please add "bag__det_remove: " to the start of the error message.
The "I" in "Item" should not be capitalized.

> +bag__intersect(A, B, Out) :-
> +	bag__init(Out0),
> +	bag__intersect_2(A, B, Out0, Out).
> +
> +:- pred bag__intersect_2(bag(T), bag(T), bag(T), bag(T)).
> +:- mode bag__intersect_2(in, in, in, out) is det.
> +bag__intersect_2(A, B, Out0, Out) :-
> +	( map__remove_smallest(A, Key, AVal,A0) ->
> +		( map__search(B, Key, BVal) ->
> +			( AVal > BVal ->
> +				map__det_insert(Out0, Key, BVal, Out1)
> +			;
> +				map__det_insert(Out0, Key, AVal, Out1)
> +			)

It would be clearer to use int__max here.

> +% deletes a file
> +:- pred io__unlink_file(string, io__res, io__state, io__state).
> +:- mode io__unlink_file(in, out, di, uo) is det.

You should name this "remove_file", and it should
be implemented using remove() rather than unlink().

> +%#include <stdio.h>
> +:- pragma(c_code, io__tmpnam(FileName::out, IO0::di, IO::uo), "{
> +	Word tmp;
> +	incr_hp_atomic(tmp, (L_tmpnam + sizeof(Word)) / sizeof(Word));
> +        tmpnam(tmp);
> +	FileName = (char *) tmp;
> +	update_io(IO0, IO);

You didn't check the return value of tmpnam().
The indentation of the call to tmpnam() is not right.

> +%#include <string.h>
> +:- pragma(c_header_code, "#include <errno.h>").
> +:- pragma(c_header_code, "#include <unistd.h>").
> +:- pragma(c_code, io__unlink_file_2(FileName::in, RetVal::out, RetStr::out), "{
> +	Word tmp;
> +	char * buf;
> +
> +	RetVal = unlink(FileName);
> +
> +	if (RetVal < 0) {
> +		buf = strerror(errno);
> +		incr_hp_atomic(tmp,(strlen(buf)+sizeof(Word)) / sizeof(Word));
> +		RetStr = (char *) tmp;
> +		strcpy(RetStr, (char *) tmp);
> +	}
> +	else
> +		RetStr = NULL;

The layout of this code does not conform to the C coding guidelines.

strerror() is not portable, unfortunately -- it doesn't work on SunOS.

Fortunately there's already code to work-around this in runtime/prof.c.
The code for it should probably go somewhere else than prof.c (e.g. a new
porting.c file) but for now we won't worry about that.  But please add
a comment to runtime/prof.c saying that strerror() is also used by
library/io.m.

You will still need to add a declaration for it.

	/*
	** if the system doesn't have strerror, we provide it
	** ourself in the Mercury runtime.
	** (It is in runtime/prof.c, currently.)
	*/
	#ifndef HAVE_STRERROR
	char *strerror(int errnum);
	#endif

This should go either in pragma_c_header code in io.m
or in one of the the runtime header files (porting.h ideally,
or in prof.h with an XXX.)

> +:- pred list__merge_and_remove_dups(pred(X, X, comparison_result)
> +	, list(X), list(X), list(X)).
> +:- mode list__merge_and_remove_dups(pred(in, in, out) is det
> +	, in, in, out) is det.

Please put the trailing commas at the end of the previous line
rather than at the start of the continuation line.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>   |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>   |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3         |     -- the last words of T. S. Garp.