[m-rev.] for review: deep profiling fixes, determinism algorithm change

[Fergus Henderson]

On 13-Aug-2002, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> +++ runtime/mercury_engine.h	2002/08/11 15:15:42
> @@ -51,8 +51,10 @@
>  #define	MR_TABLESTACKFLAG	12
>  #define	MR_UNBUFFLAG		13
>  #define	MR_AGC_FLAG 		14
> -#define	MR_DETAILFLAG		15
> -#define	MR_MAXFLAG		16
> +#define	MR_ORDINARYREGFLAG	15
> +#define	MR_ANYREGFLAG 		16
> +#define	MR_DETAILFLAG		17
> +#define	MR_MAXFLAG		18

Please s/FLAG/_FLAG/ for the new symbols.
Using LOTSOFWORDSRUNTOGETHER style is ugly and
goes against our coding guidelines.

>  /* MR_DETAILFLAG should be the last real flag */
>  
>  #define	MR_progdebug		MR_debugflag[MR_PROGFLAG]
> @@ -69,8 +71,10 @@
>  #define	MR_hashdebug		MR_debugflag[MR_TABLEHASHFLAG]
>  #define	MR_tablestackdebug	MR_debugflag[MR_TABLESTACKFLAG]
>  #define	MR_unbufdebug		MR_debugflag[MR_UNBUFFLAG]
> -#define	MR_detaildebug		MR_debugflag[MR_DETAILFLAG]
>  #define	MR_agc_debug		MR_debugflag[MR_AGC_FLAG]
> +#define	MR_ordregdebug		MR_debugflag[MR_ORDINARYREGFLAG]
> +#define	MR_anyregdebug		MR_debugflag[MR_ANYREGFLAG]
> +#define	MR_detaildebug		MR_debugflag[MR_DETAILFLAG]

Likewise here.

Also, there should be a comment above each of these,
explaining what it means.

> Index: runtime/mercury_wrapper.c
> @@ -110,8 +110,26 @@
>  
>  MR_bool		MR_check_space = MR_FALSE;
>  MR_Word		*MR_watch_addr = NULL;
> -MR_Word		*MR_watch_csd_addr = NULL;
> -int		MR_watch_csd_ignore = 0;
> +MR_CallSiteDynamic
> +		*MR_watch_csd_addr = NULL;
> +MR_bool		MR_watch_csd_started = MR_FALSE;
> +char		*MR_watch_csd_start_name = NULL;
> +
> +unsigned long	MR_lld_cur_call = 0;
> +int		MR_lld_print_enabled = 1;
> +int		MR_lld_print_name_enabled = 0;
> +int		MR_lld_print_csd_enabled = 0;
> +int		MR_lld_print_region_enabled = 0;

Use MR_TRUE and MR_FALSE rather than 1 and 0 (where appropriate).

> +char		*MR_lld_start_name = NULL;
> +unsigned	MR_lld_start_block = 100;

That magic number 100 should be explained.

> +void
> +MR_setup_call_intervals(char **more_str_ptr,
> +	unsigned long *min_ptr, unsigned long *max_ptr)
> +{
> +	char	*more_str;
> +	int	n;
> +
> +	more_str = *more_str_ptr;
> +	if (more_str == NULL || more_str[0] == '\0') {
> +		goto end;
> +	}
> +
> +	n = 0;
> +	while (MR_isdigit(more_str[0])) {
> +		n = n * 10 + more_str[0] - '0';
> +		more_str++;
> +	}
> +
> +	if (n == 0) {
> +		goto end;
> +	}
> +
> +	*min_ptr = n;
> +	if (more_str[0] == '-' && more_str[1] == '\0') {
> +		*max_ptr = (unsigned long) -1;
> +		*more_str_ptr = NULL;
> +		return;
> +	}
> +
> +	if (more_str[0] != '-') {
> +		goto end;
> +	}
> +
> +	more_str++;
> +
> +	n = 0;
> +	while (MR_isdigit(more_str[0])) {
> +		n = n * 10 + more_str[0] - '0';
> +		more_str++;
> +	}
> +
> +	if (n == 0) {
> +		goto end;
> +	}
> +
> +	*max_ptr = n;
> +	if (more_str[0] == ',') {
> +		more_str++;
> +	}
> +
> +	*more_str_ptr = more_str;
> +	return;
> +
> +end:
> +	*more_str_ptr = NULL;
> +	*min_ptr = (unsigned long) -1;
> +	*max_ptr = 0;
> +	return;
> +}

There should be a comment explaining what this function is supposed to do.

That's 60 lines of low-level C code.  Since defect rates for low-level
C code are so high, so you should probably expect it to have on average
at least six defects ;-)  I can see a couple of them: it uses `int'
when it should be using `unsigned long', and it doesn't check for
overflow.  But I'm worried about the other four ;-).
In general it's really worth trying hard to avoid long sections of
low-level C code like this.

It would probably be clearer to write this code using sscanf().
For example, something along the lines of this, which is about
half the length of the code above:

void
MR_setup_call_intervals(char **more_str_ptr,
	unsigned long *min_ptr, unsigned long *max_ptr)
{
	char		*more_str;
	unsigned long	min, max;
	int		n;

	more_str = *more_str_ptr;

	/* Relying on the return value from sscanf() with %n is
	   non-portable, so we need to call sscanf() twice here. */
	if (sscanf(more_str, "%lu-%lu", &min, &max) == 2) {
		sscanf(more_str, "%lu-%lu%n", &min, &max, &n);
		more_str += n;
		if (more_str[0] == ',') {
			more_str++;
		}
	} else if (sscanf(more_str, "%lu-", &min) == 1) {
		more_str = NULL;
		max = (unsigned long) -1;
	} else {
		more_str = NULL;
		min = 0;
		max = (unsigned long) -1;
	}

	*more_str_ptr = more_str;
	*min_ptr = min;
	*max_ptr = max;
}


Otherwise, this change looks fine.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------