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

[Fergus Henderson]

Thanks for the review, Mark.
It looks like this code needed it ;-)

On 17-Dec-1998, Mark Anthony BROWN <dougl at cs.mu.OZ.AU> wrote:
> > +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?)

Yes, it's useful.  The first argument has a different type 
to the first argument of the function it calls: `void *' v.s. `FILE *'.
But this is certainly not obvious to the reader, and ought
to be documented.  I'll replace it with the code below.

/*
** This function is just a wrapper for MR_print_proc_id_for_debugger,
** with the first argument type being `void *' rather than `FILE *',
** so that this function's address can be passed to
** MR_process_matching_procedures().
*/
static void
MR_mdb_print_proc_id(void *data, const MR_Stack_Layout_Entry *entry_layout)
{
	FILE *fp = data;
	MR_print_proc_id_for_debugger(fp, entry_layout);
}

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

It has the wrong type.

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

Good point.  I just forgot about that one.
I'll delete the XXX comment and s/MR_mdb_out/MR_mdb_err/.

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

The distinction is the same as the distinction between stdout and stderr.
MR_mdb_err is for error messages.  MR_mdb_out is for ordinary output,
including informational messages about conditions that do not constitute
errors.

I'll add a comment along these lines.

> >  	}
> > +	fprintf(MR_mdb_out, "%s ", port_name);
> 
> There is already a trailing space in port_name.

I'll delete all the trailing spaces in the strings assigned to 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)
...
> 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.

I've updated the comment describing this function 
in mercury_trace_tables.h to explain this.

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

Shall do.

I'll post a new diff shortly.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "Binaries may die
WWW: <http://www.cs.mu.oz.au/~fjh>  |   but source code lives forever"
PGP: finger fjh at 128.250.37.3        |     -- leaked Microsoft memo.