[m-dev.] for review: new debugger command set (part 4 of 4)

[Tyson Dowd]

On 01-Oct-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:

> Index: trace/mercury_trace_internal.c
> <the whole file is smaller and much more readable than the diff>
> IMPLEMENT NEW DEBUGGER COMMAND SET
> SUPPORT RETRY
> ADD REDO EVENTS
> ADD TRACE DEPTH HISTOGRAMS
> /*
> ** 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.
> */
> 
> /*
> ** This file contains the code of the internal, in-process debugger.
> **
> ** Main author: Zoltan Somogyi.
> */
> 
> #include "mercury_imp.h"
> #include "mercury_trace.h"
> #include "mercury_trace_internal.h"
> #include "mercury_trace_alias.h"
> #include "mercury_trace_help.h"
> #include "mercury_trace_spy.h"
> #include "mercury_trace_tables.h"
> #include "mercury_layout_util.h"
> #include "mercury_macros.h"
> #include "mercury_getopt.h"
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <ctype.h>
> 
> /* The initial size of the spy points table. */
> #define	MR_INIT_SPY_POINTS	10
> 
> /* The initial size of arrays of argument values. */
> #define	MR_INIT_ARG_COUNT	20
> 
> /* The initial size of arrays of words. */
> #define	MR_INIT_WORD_COUNT	20
> 
> /* The initial size of arrays of characters. */
> #define	MR_INIT_BUF_LEN		80
> 

Except for the other initial size of arrays of characters,
MR_NUMBER_LEN, which is defined 1666 lines further down this
file.

In my last review I said:

   Why define this here?  You have an 80 character maximum buffer length
   definition at the top of the file.

This comment was not addressed.

Is this because the MR_INIT_BUF_LEN is just an initial length and
is changable, whereas (for some reason) MR_NUMBER_LEN uses a fixed
limit?  At any rate, I don't know why MR_NUMBER_LEN is all the
way down in the file where it will be easily overlooked.

> /* The initial number of lines in documentation entries. */
> #define	MR_INIT_DOC_CHARS	800
> 
> #define	MDBRC_FILENAME		".mdbrc"
> #define	DEFAULT_MDBRC_FILENAME	"mdbrc"
> 
> /* XXX We should consider whether the following should be thread local. */
> static	MR_Spy_Point    	**MR_spy_points;
> static	int			MR_spy_point_next = 0;
> static	int			MR_spy_point_max  = 0;
> 
> static	MR_Trace_Print_Level	MR_default_print_level = MR_PRINT_LEVEL_SOME;
> 

Line should be wrapped.

> 			printf("The default print level is ");
> 			switch (MR_default_print_level) {
> 				case MR_PRINT_LEVEL_NONE:
> 					printf("none.\n");
> 					break;
> 				case MR_PRINT_LEVEL_SOME:
> 					printf("some.\n");
> 					break;
> 				case MR_PRINT_LEVEL_ALL:
> 					printf("all.\n");
> 					break;
> 				default:
> 					printf("something weird.\n");
> 					break;

Better error message or an XXX.

> 	} else if (streq(words[0], "echo")) {
> 		if (word_count == 2) {
> 			if (streq(words[1], "off")) {
> 				MR_echo_commands = FALSE;
> 				printf("Command echo disabled\n");
> 			} else if (streq(words[1], "on")) {
> 				MR_echo_commands = TRUE;
> 				printf("Command echo enabled\n");

Sometimes there is a full stop at the end of responses, sometimes
there isn't.  I don't care much which happens, but I do like
consistency.

> 
> /*
> ** Given a text line, break it up into words composed of non-space characters
> ** separated by space characters. Make each word a NULL-terminated string,
> ** overwriting some spaces in the line array in the process.
> **
> ** If the first word is a number but the second is not, swap the two.
> ** If the first word has a number prefix, separate it out.
> **
> ** On return *words will point to an array of strings, with space for
> ** *words_max strings. The number of strings (words) filled in will be
> ** given by the return value.

This isn't true, the return value is a char *, not an int.

> static const char *
> MR_trace_parse_line(char *line, char ***words, int *word_max, int *word_count)

It appears to actually be an error message that is returned.

> static void
> MR_trace_source(const char *filename)
> {
> 	FILE	*fp;
> 
> 	if ((fp = fopen(filename, "r")) != NULL) {
> 		MR_trace_source_from_open_file(fp);
> 		fclose(fp);
> 	} else {
> 		perror(filename);
> 		/*
> 		** We actually want to write to stdout, but the following
> 		** is too Unix-specific:
> 

An XXX on this statement would be good -- it could be fixed if it
was felt to be worthwhile at some stage in the future.

> +static int
> +MR_search_spy_table_for_proc(const MR_Stack_Layout_Entry *entry)
> +{
> +	int				lo;
> +	int				hi;
> +	int				mid;
> +
> +	lo = 0;
> +	hi = MR_spied_proc_next - 1;
> +	while (lo <= hi) {
> +		mid = (lo + hi) / 2;
> +		if (MR_spied_procs[mid].spy_proc == entry) {
> +			return mid;
> +		} else if (MR_spied_procs[mid].spy_proc < entry) {
> +			lo = mid + 1;
> +		} else {
> +			hi = mid - 1;
> +		}
> +	}
> +
> +	return -1;
> +}

Why don't you use the MR_bsearch macro to perform the search?

This comment was also made in the last review, but was unaddressed.

> +static bool MR_need_move(int i, const MR_Stack_Layout_Entry *entry);
> +

Should this be here?

Unless the file is clearly sectioned, I don't see any reason to put
prototypes in the middle instead of up the top with the others..

> +
> +typedef enum {
> +	MR_SPY_PRINT, MR_SPY_STOP
> +} MR_Spy_Action;
> +
> +#define	MR_spy_action_string(a)		((a == MR_SPY_STOP) ? "stop" :      \
> +					(a == MR_SPY_PRINT) ? "print" :     \
> +					"weird spy action")
> +

Like the error message earlier, I think weird is a bit cryptic.
"unknown" would do just as well and would correctly indicate what is
going on.


Apart from that it seems OK.

-- 
Those who would give up essential liberty to purchase a little temporary
safety deserve neither liberty nor safety.     - Benjamin Franklin

Tyson Dowd   <tyson at tyse.net>   http://tyse.net