[m-rev.] for review: extras/quickcheck comments

[Peter Moulder]

The attached diff currently only adds comments and doesn't change any
code.  The reviewer may suggest acting on some of these comments.

I won't write up a commit message until it's decided which of the
comments should be acted on.  The current (comments-only) diff is
fairly short.

pjm.


Index: qcheck.m
===================================================================
RCS file: /home/mercury1/repository/mercury/extras/quickcheck/qcheck.m,v
retrieving revision 1.2
diff -d -u -r1.2 qcheck.m
--- qcheck.m	31 Jul 2001 13:47:32 -0000	1.2
+++ qcheck.m	22 Feb 2002 04:08:23 -0000
@@ -815,20 +815,54 @@
 	% generate int
 rand_int(BS0, BS) = Int :-
 	Temp = rand_allint(BS0, BS1) rem 2,
+	% effic: rnd(Temp, BS0, BS1), test Temp < 0.5.
+	% (Or, for that matter, introduce a predicate that
+	% avoids the float conversion.)
 	(if	Temp = 0 
 	 then
 		irange(-100, 100, Int, BS1, BS)
 	 else
+		% If we test the extremes of int at
+		% all, then I [pjm] suggest we should increase
+		% the probability of int__max_int and
+		% int__min_int, since these values are very
+		% subject to bugs (e.g. due to assuming that X + 1 > X).
+		% You may say that it's often OK for a predicate not
+		% to work for such "extreme" values; but for such predicates
+		% I think the user ought at least be aware that the bug
+		% exists (e.g. they may need to document the bug even if
+		% they decide the predicate doesn't need fixing).
+		% The cost of increasing the probability of extreme values
+		% is forcing the user to specify a different probability
+		% distribution; not too big a deal.
+
+		% If you don't think we should increase the probability
+		% of choosing int__max_int/int__min_int, then consider
+		% instead making them impossible.
+
+		% (In any case, make sure that the description of rand_int
+		% in T5.html is correct.)
+
 		Int = rand_allint(BS1, BS)
 	).  
 
 	% generate int
 rand_allint(BS0, BS) = Int :-
 	next(1, Sign, BS0, BS1),
+	% XXX: Isn't it possible for Mercury int type to be wider than
+	% 32 bits?  In which case it's probably fairly important to
+	% test more than just [-2^31, 2^31).
+	% Note when fixing this that rnd is implemented in terms of 
+	% float, which has only 53 bits of precision, so need to
+	% call rnd multiple times.
 	next(31, TempInt, BS1, BS),
 	( Sign > 0 ->
 	    Int =  TempInt 
 	;
+	    % XXX: Shouldn't this be `Int = \TempInt'?
+	    % The existing code over-represents 0 and
+	    % doesn't test -2^31.  In either case,
+	    % should fix T5.html to correspond.
 	    Int = -TempInt
 	).
 
@@ -842,6 +876,21 @@
 		 else
 		 	Char = rand_char(RS1, RS)
 		).
+/* Suggested replacement implementation, using the actual bounds rather
+   than the arbitrary [-999, 999] (sic) range used above:
+
+	char__min_char_value(Min),
+	char__max_char_value(Max),
+	irange(Min, Max, Int, RS0, RS),
+	(char__to_int(Char0, Int) ->
+		Char = Char0
+	;
+		error("can't happen: bad int to char conversion")
+	).
+
+   If this replacement is used, then update the description of
+   rand_char in T5.html.
+*/
 
 	% generate float
 rand_float(BS0, BS) = Flt :-
Index: rnd.m
===================================================================
RCS file: /home/mercury1/repository/mercury/extras/quickcheck/rnd.m,v
retrieving revision 1.1
diff -d -u -r1.1 rnd.m
--- rnd.m	31 May 2001 02:08:45 -0000	1.1
+++ rnd.m	22 Feb 2002 04:08:23 -0000
@@ -61,13 +61,21 @@
 :- import_module math, require.
 
 irange(Min, Max, Val, R0, R) :-
+	% XXX: Should either document that Max must not be int__max_int,
+	% or change the below to rfloat(Max)+1.
 	frange(rfloat(Min), rfloat(Max+1), FVal, R0, R),
+	% XXX: The documentation says that irange is evenly
+	% distributed, in which case should use floor rather
+	% than rint.
 	Val = rint(FVal).
 
 frange(Min, Max, Val, R0, R) :-
 	rnd(J, R0, R),
 	Val = J*(Max - Min)+Min.
 
+% XXX: Should probably call random__permutation instead, which does a
+% "complete" shuffle; the existing implementation of shuffle reorders
+% at most 6 elements.
 shuffle(Ins, Outs, R0, R) :-
 	list__length(Ins, N),
 	shuffle2(N, Ins, [], T0, R0, R1),
@@ -80,6 +88,8 @@
 :- pred shuffle2(int, list(T), list(T), list(T), rnd, rnd).
 :- mode shuffle2(in, in, in, out, in, out) is det.
 
+% Pick a random element (uniform distribution) and move it to
+% the beginning of the list.
 shuffle2(N, Ins, Acc0, Acc, R0, R) :-
 	( N > 0 ->
 		irange(0, N-1, J, R0, R1),
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------