[m-dev.] For review: implementation of collect for Opium-M

[Fergus Henderson]

On 02-Nov-1999, Erwan Jahier <Erwan.Jahier at irisa.fr> wrote:
> | (Another difference with foldl/4 is that it stops when the filter returns `no'.
> | In fact it's a bit more like `std_util__do_while', the only difference is that
> | it operators on a sequence of debugger events rather than a sequence of solutions.
> 
> But I am not sure that std_util__do_while is as famous as fold/4. Is it?

No.

> If it is not the case, I prefer to explain the collect in terms of fold/4.

OK.  But perhaps s/fold/foldl/ ?  At least in Mercury and Haskell,
the function is known as `foldl' rather than `fold'.

> | > browser/debugger_interface.m:
> | ...
> | > 	Define a new function get_collecting_variable_type/2 that retrieves
> | > 	the type of a variable. This type is needed in MR_trace_event_external()
> | > 	to be able to call MR_make_permanent() on MR_collecting_variable; we
> | > 	need to do that to ensure that the memory allocated for it won't be 
> | > 	deallocated on backtracking.
> | 
> | I think that function should not be needed anymore.
> 
> Why? I introduced it in my last change to be able to use MR_make_permanent()!
> Do you mean ML_DI_link_collect() should link with the collecting variable type 
> rather than a function that is able to retrieve its type?

I meant that that function should not be needed in browser/debugger_interface.m,
since it is now in collect.in instead.  The log message seems to be out of date.

> Index: 0.6/debugger_interface.m
> --- 0.6/debugger_interface.m Tue, 02 Nov 1999 08:31:18 +0100 jahier (collect/1_debugger_i 1.4 640)
> +++ 0.6(w)/debugger_interface.m Tue, 02 Nov 1999 11:13:01 +0100 jahier (collect/1_debugger_i 1.4 640)
> @@ -283,6 +283,9 @@
>  	;	grade(string)
>  	% responses to collect
>  	%;	collected(collected_type)
> +		% It is commented because collected_type is unknow at compile 
> +		% time since it is defined by by users in the dynamically 
> +		% linked collect module.

s/It is/This is/
s/commented/commented out/
s/unknow/unknown/
s/by by/by/

> Index: 0.6/mercury_trace_external.c
> -				collect_linked = (result =  'y');
> +				collect_linked = (result = 'y');

That's a bug: the second `=' should be `=='!

> +:- module collect_lib.
> +:- interface.
> +:- import_module io, char, dl.
> +
> +% dynamically link the collect module; 
> +:- pred link_collect(string, c_pointer, c_pointer, c_pointer, c_pointer, 
> +	dl__result(handle), char, io__state, io__state).
> +:- mode link_collect(in, out, out, out, out, out, out, di, uo) is det.

According to our Mercury coding guidelines, the comments in the interface
should be sufficient for someone to understand how to call a procedure
without having to look at the implementation.
So the comment here should explain the meaning of the parameters.

> +% interface to the C function dlclose()
> +:- pred close_collect(dl__result(handle), io__state, io__state).
> +:- mode close_collect(in, di, uo) is det.

I suggest that you change the comment for that one to

	% Dynamically unlink a module that was dynamically linked in
	% using `link_collect'.

and change the name from `close_collect' to `unlink_collect'.

The point of the name change is that `link_collect' and `close_collect'
are a pair of related routines that should always be called together,
so they should be given corresponding names, e.g. `link' and `unlink'
or `open' and `close'.

> --- mercury_trace_external.c	1999/10/28 20:15:02	1.27
> +++ mercury_trace_external.c	1999/11/02 10:14:12
>  /*
> +** Type of a local variable that indicates in which mode the external 
> +** debugger is. When the external debugger is in mode:
> +** (1) `MR_searching', it tries to find an event that matches a forward 
> +**      move request,
> +** (2) `MR_reading_request', it reads a new request on the socket,
> +** (3) `MR_collecting', it is collecting information (after a `collect' request).
> +*/
> +typedef enum {
> +	MR_searching, MR_reading_request, MR_collecting
> +} MR_external_debugger_mode_type;
> +
> +static	MR_external_debugger_mode_type 
> +	external_debugger_mode = MR_reading_request;

s/local variable/static variable/

("local" in "local variable" normally means "local to a function"
rather than "local to a translation unit".)

> +		case MR_collecting:
> +			(*send_collect_result_ptr)(
> +				MR_collecting_variable, 
> +				(Word) &MR_debugger_socket_out);

I think that is a bug: shouldn't that call be enclosed inside
MR_TRACE_CALL_MERCURY()?

When you've addressed those comments, I'd like to see another diff,
again in both relative and full versions.  Thanks.

-- 
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.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------