[m-rev.] for review: new funcs and preds in the bag module

[Julien Fischer]

On Thu, 8 Mar 2007, Ondrej Bojar wrote:

> An easy review for anyone. (Peter? Ralph?)
>
> Estimated hours taken: 0.75
> Branch: main
>
> Add functions bag.count/1, bag.unique_elements/1 and predicates bag.member/2
> and bag.member/3 to the bag module.
>
> NEWS:
>    Announce the added predicates and functions.

s/added/new/

>    Minor indentation unification.
>
> library/bag.m:
>    New functions and predicates.
>
> tests/hard_coded/bag_various.m:
> tests/hard_coded/bag_various.exp:
>    A simple testcase for the new predicates, with expected results.
>
> tests/hard_coded/Mmakefile:
>    Enabled the test.


>
>
> Index: NEWS
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/NEWS,v
> retrieving revision 1.450
> diff -u -r1.450 NEWS
> --- NEWS	6 Mar 2007 04:22:37 -0000	1.450
> +++ NEWS	8 Mar 2007 05:28:37 -0000
> @@ -48,11 +48,17 @@
>    determinism multi.
>
> * The following functions have been added to the string module:
> -        string.split_at_separator/2
> -        string.split_at_char/2
> -        string.split_at_string/2
> +	string.split_at_separator/2
> +	string.split_at_char/2
> +	string.split_at_string/2
> 	string.remove_suffix_if_present/2
>
> +* The following functions and predicates have been added to the bag module:
> +	bag.count/1
> +	bag.unique_elements/1
> +	bag.member/2
> +	bag.member/3
> +
> * We have changed the interface of the ops module to make lookups of 
> operators
>   more efficient.
>
> Index: library/bag.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/library/bag.m,v
> retrieving revision 1.31
> diff -u -r1.31 bag.m
> --- library/bag.m	23 Oct 2006 00:32:55 -0000	1.31
> +++ library/bag.m	8 Mar 2007 05:28:37 -0000
> @@ -31,6 +31,16 @@
> :- pred bag.init(bag(T)::out) is det.
> :- func bag.init = bag(T).
>
> +    % Return the number of elements in a bag, duplicit elements are counted
> +    % as many times as they occur in the bag.
> +    %

I suggest:

 	Return the number of values in the bag (including duplicate
 	values).


> +:- func bag.count(bag(T)) = int.
> +
> +    % Return the number of unique elements in a bag, duplicit elements are
> +    % counted only once.
> +    %


You mean "duplicate" rather than "duplicit" here (and in other spots).
The latter means something different.

> +:- func bag.unique_elements(bag(T)) = int.

I suggest renaming this to bag.count_unique/1.

> +
>     % Insert a particular value in a bag.
>     %
> :- pred bag.insert(bag(T)::in, T::in, bag(T)::out) is det.
> @@ -46,6 +56,18 @@
> :- pred bag.insert_set(bag(T)::in, set(T)::in, bag(T)::out) is det.
> :- func bag.insert_set(bag(T), set(T)) = bag(T).
>
> +    % bag.member(Elem, Bag) :
> +    %   True iff `Bag' contains at least one occurrence of `Elem'.
> +    %
> +:- pred bag.member(T::in, bag(T)::in) is semidet.

Note that the rest of the documentation in the bag module refers to 
`values' rather than `elements'.  For consistency you should do the
same here as well.

> +
> +    % bag.member(Elem, Bag, Remainder) :
> +    %   Nondeterministically returns all elements from Bag and the
> +    %   corresponding bag after the element removed. Duplicit items are
> +    %   returned as many times as they occur in the Bag.
> +    %

 	... after the value has been removed.

> +:- pred bag.member(T::out, bag(T)::in, bag(T)::out) is nondet.
> +
>     % Make a bag from a list.
>     %
> :- func bag.bag(list(T)) = bag(T).
> @@ -237,6 +259,18 @@
>
> %---------------------------------------------------------------------------%
>
> +bag.count(Bag) =
> +    list.foldl(func(A, B) = A+B,
> +        map.values(Bag),
> +        0
> +    ).

Just use int.plus/2 instead of the above closure.

> +%---------------------------------------------------------------------------%
> +
> +bag.unique_elements(Bag) = map.count(Bag).
> +
> +%---------------------------------------------------------------------------%
> +
> bag.insert(Bag0, Item, Bag) :-
>     ( map.search(Bag0, Item, Count0) ->
>         Count = Count0 + 1
> @@ -257,6 +291,13 @@
>     % XXX We should exploit the sortedness of List.
>     bag.insert_list(Bag0, List, Bag).
>
> +bag.member(M, Bag) :- map.search(Bag, M, _Occurrences).
> +

Put the body of this predicate on its own line.

> +bag.member(OutElem, InBag, OutBag) :-
> +  Elems = bag.to_list(InBag),
> +  list.member(OutElem, Elems),
> +  OutBag = bag.det_remove(InBag, OutElem).
> +
> bag.from_list(List, Bag) :-
>     bag.init(Bag0),
>     bag.insert_list(Bag0, List, Bag).


The rest looks okay.

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