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

[Fergus Henderson]

> *** RECAST MERCURY_TRACE_UTIL AS MERCURY_LAYOUT_UTIL ***
> 
> runtime/mercury_trace_util.[ch]:
> runtime/mercury_layout_util.[ch]:
> 	Rename this module from trace_util to layout_util, since it is also
> 	used by the native garbage collector. Remove "trace" from the names
> 	of functions.
> 
> 	Get rid of the global variable MR_saved_regs, and instead thread
> 	a pointer to this data structure through the relevant functions
> 	as an extra argument.
> 
> 	Add a lot more documentation in the header file.
> 
> 	Add an extra function, MR_get_register_number, for use by retry.

Hmm, that last line seems to be describing something that should
be part of the change to add "retry", rather than as part of this
change.

> runtime/Mmakefile:
> 	Reflect the module rename.
> 
> runtime/*.c:
> 	Refer to the new module.
...
> Index: runtime/mercury_accurate_gc.c
> @@ -87,8 +92,8 @@
>  		var_count = layout->MR_sll_var_count;
>  		vars = &(layout->MR_sll_var_info);
>  
> -		type_params = MR_trace_materialize_typeinfos_base(vars,
> -			top_frame, stack_pointer, current_frame);
> +		type_params = MR_materialize_typeinfos_base(vars,
> +			NULL, stack_pointer, current_frame);

Why do you pass NULL here?  I think a comment here would help.

> -			if (MR_trace_get_type_and_value_base(&sl_var, 
> -					top_frame, stack_pointer,
> +			if (MR_get_type_and_value_base(&sl_var, 
> +					NULL, stack_pointer,
>  					current_frame, type_params,
>  					&type_info, &value)) {

Ditto.

>  				printf("\t");
> -				MR_trace_write_variable(type_info, value);
> +				MR_write_variable(type_info, value);
>  				printf("\n");

Which stream does `MR_write_variable' write to?
I guess it had better be `stdout', otherwise those printf() calls
won't match it.

> +MR_materialize_typeinfos_base(const MR_Stack_Layout_Vars *vars,
> +	Word *saved_regs, Word *base_sp, Word *base_curfr)
...
> -				if (!succeeded) {
> +				if (! succeeded) {

Personally I find the former more readable. 
Particularly when it comes to more complicated things like `!foo ||
bar' vs. `! foo || bar' where using spaces around the unary operators
makes the precedence seem more ambiguous.

A quick grep through the source code suggests that `!foo' is used more
commonly than `! foo', so the above change just increases the level of
inconsistency.

> -		if (!MR_trace_get_type_and_value_filtered(var, name, 
> +		if (! MR_get_type_and_value_filtered(var, saved_regs,

Ditto ('!' vs '! ').

> Index: trace/mercury_trace.h
...
> -		if (!MR_trace_get_type_filtered(var, name, &type_info))
> +		if (! MR_get_type_filtered(var, saved_regs, name, &type_info))

Ditto ('!' vs '! ').


[I haven't finished reviewing this one yet.]

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