[m-dev.] for review: new browser parameters

[Zoltan Somogyi]

On 19-Oct-2000, Mark Anthony BROWN <dougl at cs.mu.OZ.AU> wrote:
> browser/browser_info.m:
> 	New module which defines the key browser data structures.  The
> 	`browser_state' previously used by browse.m has been split into
> 	two parts: the transient state (browser_info) and the persistent
> 	state (browser_state).  The persistent state is created when the
> 	browser is first called, and is saved between calls to the browser,
> 	whereas a new transient state is created each time the browser is
> 	called, and lasts for the duration of the call.

The name browser_state suggests something transient to me. We probably
shouldn't use browser_params, since the values stored in it can be overridden.
How about browser_preferences?

> @@ -353,13 +307,13 @@
>  "\tquit           -- quit browser\n",
>  "SICStus Prolog style commands are:\n",
>  "\tp              -- print\n",
> -"\t< [n]          -- set depth (default is ", DefaultStr, ")\n",
> +"\t< [n]          -- set depth\n",

What happens now if you don't supply a number?

> +	% The maximum estimated size for which we use `io__write'.
> +:- pred max_print_size(int::out) is det.
> +max_print_size(60).

Any reason for not using a function?

> +	% Estimate the total term size, in characters.
> +	% We count the number of characters in the functor,
> +	% plus two characters for each argument: "(" and ")"
> +	% for the first, and ", " for each of the rest,
> +	% plus the sizes of the arguments themselves.
> +	% This is only approximate since it doesn't take into
> +	% account all the special cases such as operators.
> +:- pred term_size(univ::in, int::out) is det.
> +term_size(Univ, TotalSize) :-
> +	deconstruct(Univ, Functor, Arity, Args),
> +	string__length(Functor, FunctorSize),
> +	list__map(term_size, Args, ArgSizes),
> +	AddSizes = (pred(X::in, Y::in, Z::out) is det :- Z = X + Y),
> +	list__foldl(AddSizes, ArgSizes, Arity * 2, TotalArgsSize),
> +	TotalSize = TotalArgsSize + FunctorSize.

Is this still used anywhere? Please make sure it isn't, since it can take
a looonnnggg time on large terms.

> +The browser also maintains separate configuration parameters
> +for the three different output formats.
> +The options @samp{-f} or @samp{--flat}, @samp{-p} or @samp{--pretty},
> +and @samp{-v} or @samp{--verbose}
> +select which formats will be affected by the change.
> +If none of these options is given,
> +the default is to affect all formats.

You should mention that the one exception from this is if you set
the format itself.

> +static	bool		MR_trace_is_portray_format(const char *str,
> +				MR_Browse_Format *format);

This indentation looks incorrect.

> +	else if (streq(param, "format") &&
> +			MR_trace_is_portray_format(value, &new_format))
> +	{
> +		MR_TRACE_CALL_MERCURY(
> +			ML_BROWSE_set_param_format(print, browse, print_all,
> +				new_format, MR_trace_browser_state,
> +				&MR_trace_browser_state);
> +		);

I would put this code either first or last in the chain of if-then-elses,
since it is somewhat different from the others.

> -extern 	void	MR_trace_browse(MR_Word type_info, MR_Word value);
> +extern 	void	MR_trace_browse(MR_Word type_info, MR_Word value,
> +			MR_Browse_Format format);

Indentation looks suspect here and in some places following it.

> New file: browser/browser_info.m:

I think CVS has been updated on all the machines we use, except possibly
mundook, to a version that can handle removed as well as added files
gracefully.

> 			dirs	:: list(dir),		% Root rel `present
> 							% working directory'.

This comment is not clear: it doesn't say whether the list starts at the root
or at the cwd.

> :- type browse_caller_type
> 	--->	print		% Non-interactively called via mdb's `print'
> 				% command, to print a single value.
> 	;	browse		% Interactively called via mdb's `browse'
> 				% command.
> 	;	print_all.	% Non-interactively called via mdb's `print *'
> 				% command, to print one of a sequence of
> 				% values.

The ordering here is a bit strange.

> :- type setting
> 	--->	depth(int)
> 	;	size(int)
> 	;	format(portray_format)
> 	;	width(int)
> 	;	lines(int)
> 	.

In your other types, the dot is not on a separate line.

> :- pred set_param_depth(bool, bool, bool, bool, bool, bool, int,
> 		browser_state, browser_state).
> :- mode set_param_depth(in, in, in, in, in, in, in, in, out) is det.

I find that using predmode declarations make working with code like this
significantly easier.

> browser_info__get_format(Info, Caller, MaybeFormat, Format) :-
> 	(
> 		MaybeFormat = yes(Format)
> 	;
> 		MaybeFormat = no,
> 		Override = Info ^ format,
> 		(
> 			Override = yes(Format)
> 		;
> 			Override = no,
> 			get_caller_params(Info ^ state, Caller, Params),
> 			Format = Params ^ default_format
> 		)
> 	).

The variable Override is misnamed, since it doesn't override the parameter
supplied by the user for this command.

> 	% Initialise the persistent settings with default values.  The
> 	% rationale for the default values is:
> 	% 	Depth and Size:
> 	%		For non-interactive display, these are 3 and 10 resp.,
> 	%		so that terms will generally fit on one line.  For
> 	%		interactive browsing these values are increased.
> 	%
> 	%	Width:
> 	%		Defaults to 80 characters in any situation.
> 	%
> 	%	Lines:
> 	%		If one term is printed then it is limited to 25 lines.
> 	%		If there can be more than one term (i.e., with
> 	%		`print *') then a much lower limit is imposed.  For
> 	%		verbose format, there is not much point setting this to
> 	%		less than about 5 since otherwise very little of the
> 	%		term will be shown.

I think the default depth and size limits for printing one variable should be
significantly greater than 3 and 10; they should probably be the same as the
corresponding defaults for browsing.

Otherwise, the diff looks good.

Zoltan.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------