[m-dev.] for review: subdivision of mercury_trace.c

[Fergus Henderson]

On 09-Apr-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> This ought to make further development easier.

Yes, thanks.

> runtime/mercury_trace.[ch]:
> runtime/mercury_trace_cmd.h:
> runtime/mercury_trace_internal.[ch]:
> runtime/mercury_trace_external.[ch]:
> runtime/mercury_trace_util.[ch]:
> 	Divide the old mercury_trace.c into several components, with one
> 	module for the internal debugger, one for the interface to the
> 	external debugger, one for utilities needed by both. Mercury_trace.c
> 	now has only the top-level stuff that steers between the two
> 	debuggers. It now has two interface files: mercury_trace.h (which
> 	is unchanged) to compiled code and mercury_trace_cmd.h to allow
> 	the internal debugger module to control how events are handled.

Having two header files for one C file sort of goes against our
C coding guidelines.  I understand the rationale in this case.
However, you could make mercury_trace_cmd.h a module of its own,
with mercury_trace_cmd.c containing only the definitions for the
global variables declared in mercury_trace_cmd.h.
This might be a better approach.

Comments?


Something else that could usefully go with this change is to rename
MR_trace_display_user() as MR_trace_event_internal()
and MR_trace_debugger_step() as MR_trace_event_external().

> ::::::::::::::
> mercury_trace_external.c
> ::::::::::::::
... some comments ...
> #ifdef MR_USE_EXTERNAL_DEBUGGER
... all the code ...
> #endif /* MR_USE_EXTERNAL_DEBUGGER */

ANSI C forbids source file which are empty after preprocessing.
This is a dumb restriction, but we should probably respect it.
I'm not sure what the best fix is -- perhaps just moving the
#includes out of the #ifdef.

> ::::::::::::::
> mercury_trace_external.h
> ::::::::::::::
> /*
> ** Copyright (C) 1998 The University of Melbourne.
> ** This file may only be copied under the terms of the GNU Library General
> ** Public License - see the file COPYING.LIB in the Mercury distribution.
> */
> 
> #ifndef	MERCURY_TRACE_EXTERNAL_H
> #ifdef	MR_USE_EXTERNAL_DEBUGGER
> 
> extern	void	MR_trace_init_external(void);
> extern	void	MR_trace_end_external(void);
> extern	void	MR_debugger_step(MR_trace_interact interact,
> 			const MR_Stack_Layout_Label *layout,
> 			MR_trace_port port, int seqno, int depth,
> 			const char *path);
> 
> #endif	/* MR_USE_EXTERNAL_DEBUGGER */
> #endif	/* MERCURY_TRACE_EXTERNAL_H */

There should be a #define MERCURY_TRACE_EXTERNAL_H after the #ifndef. 
(Also a blank line between the two #ifs and #endifs might help.)

In mercury_trace_internal.h:
> static void
> MR_list_spy_points(void)
> {
> 	int	i;
> 
> 	for (i = 0; i < MR_next_spy_point; i++) {
> 		printf("%2d: %s %s:%s\n", i,
> 			MR_spy_points[i].enabled? "+": "-",

This is something I missed last time:  I would find this easier to read
if there was a space before the ? and :.

> ::::::::::::::
> mercury_trace_util.h
> ::::::::::::::
> /*
> ** Copyright (C) 1998 The University of Melbourne.
> ** This file may only be copied under the terms of the GNU Library General
> ** Public License - see the file COPYING.LIB in the Mercury distribution.
> */
> 
> #ifndef	MERCURY_TRACE_UTIL_H

Please add

  #define	MERCURY_TRACE_UTIL_H

after the #ifndef.

Otherwise, that looks fine, so once you've addressed the above issues
please commit it.

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