[m-dev.] for review: mdb I/O streams

[Mark Anthony BROWN]

Hello,

Fergus Henderson writes:
> 
> Estimated hours taken: 3
> 
> Add support to mdb for doing the debugger I/O in a different
> window to the I/O of the program being debugged.
> 
> XXX TODO: I/O from the browser still goes to the standard I/O streams
> rather than the debugger I/O streams.
> 
> runtime/mercury_wrapper.h:
> runtime/mercury_wrapper.c:
> 	Add new runtime options for specifying filenames
> 	for the mdb I/O streams.
> 
> doc/user_guide.texi:
> 	Document the new optinos.
> 
> trace/mercury_trace_internal.c:
> 	Add three new variables holding the mdb I/O streams.
> 	Initialise them based on the option settings.
> 	Change the code so that all I/O goes through these streams.
> 	Also abstract a few bits of duplicated code into separate functions.
> 
> trace/mercury_trace_tables.c:
> trace/mercury_trace_tables.h:
> 	Pass a `void *' parameter through MR_process_matching_procedures()
> 	to the function it calls.  This is needed by mercury_trace_internal.c
> 	to pass `MR_mdb_in' down to MR_print_proc_id_for_debugger().
> 
> runtime/mercury_stack_trace.c:
> runtime/mercury_stack_trace.h:
> 	Add a `FILE *' parameter to MR_print_proc_id_for_debugger().
> 


> +static void
> +MR_mdb_print_proc_id(void *fp, const MR_Stack_Layout_Entry *entry_layout)
> +{
> +	MR_print_proc_id_for_debugger(fp, entry_layout);
> +}
> +

Is this function really useful?  (Am I missing something?)

> +					fprintf(MR_mdb_err,
> +						"Ambiguous procedure "
>  						"specification. "
>  						"The matches are:\n");
> +					/* XXX mdb_err vs mdb_out */
>  					MR_process_matching_procedures(&spec,
> -						MR_print_proc_id_for_debugger);
> +						MR_mdb_print_proc_id,
> +						MR_mdb_out);

... what is wrong with passing MR_print_proc_id_for_debugger here?

Also, half the output of the last message goes to MR_mdb_err and the
rest goes to MR_mdb_out.  Is that what the XXX is for?  Why not just
fix it?

>  			} else {
> -				printf("Break point #%d does not exist.\n", n);
> +				fprintf(MR_mdb_err,
> +					"Break point #%d does not exist.\n",
> +					n);

Some of these sort of messages go to MR_mdb_out, others go to
MR_mdb_err.  What precisely are the streams intended to be used for?


>  	if (print_value) {
>  		if (browse) {
> +			/* XXX shoudl use MR_mdb_in and MR_mdb_out */

s/shoudl/should/

> +			fprintf(MR_mdb_out, "\t");
> +			fflush(MR_mdb_out);
> +			/* XXX shoudl use MR_mdb_out */

...and again

>  	}
> +	fprintf(MR_mdb_out, "%s ", port_name);

There is already a trailing space in port_name.


>  MR_process_matching_procedures(MR_Proc_Spec *spec,
> -	void f(const MR_Stack_Layout_Entry *))
> +	void f(void *, const MR_Stack_Layout_Entry *),
> +	void *data)
>  {
>  	if (spec->MR_proc_module != NULL) {
>  		MR_Module_Info			*module;
> @@ -332,14 +338,14 @@
>  		module = MR_search_module_info(spec->MR_proc_module);
>  		if (module != NULL) {
>  			MR_process_matching_procedures_in_module(
> -				module, spec, f);
> +				module, spec, f, data);

Oh, I get why the first argument to f is a void * now.  I
think a comment mentioning how that argument is used would
be helpful, though.


Overall, the changes are fine.  However, I think it should be
documented somewhere what types of messages should be sent
to MR_mdb_out, and what should be sent to MR_mdb_err.

Cheers,
Mark
-- 
Mark Brown  (dougl at cs.mu.oz.au)       )O+   |  For Microsoft to win,
MEngSc student,                             |  the customer must lose
Dept of Computer Science, Melbourne Uni     |          -- Eric S. Raymond