[m-dev.] for review: term browser (version one)

[Fergus Henderson]

On 14-Oct-1998, Bert THOMPSON <aet at cs.mu.OZ.AU> wrote:
> Add a simple term browser for use by the trace-based debugger.
...
> N.B. This still needs to be hooked into the debugger.
...
> +++ browser_library.m	1998/10/14 15:16:26
> @@ -14,6 +14,10 @@
>  
>  :- import_module help.
>  :- import_module debugger_interface.
> +:- import_module browse.
> +:- import_module frame.
> +:- import_module parse.
> +:- import_module util.

This is a bit of a side issue, but I should point out that
these module names will invade the user's module name space;
the user won't be able to use a module named `util', for example.
To solve this, these modules ought to be submodules of the
`browser_library' module (which should perhaps be renamed as just
`browser').

But nested modules still have a few problems at the moment,
so leave it as is.

> :- module browse.
> 
> :- interface.
> 
> :- import_module io, std_util.
> 
> :- pred browse__browser(univ, io__state, io__state).
> :- mode browse__browser(in, di, uo) is det.
> 
> %---------------------------------------------------------------------------%
> :- implementation.

Probably it would be better to name the routine `browse' (a verb)
rather than `browser' (a noun).

I think we really need two different interfaces to the browser:
an interactive one, and a non-interactive one which just displays
terms with some kind of fixed depth limit.

> 	browser2(State).
> 
> :- pred browser2(browser_state, io__state, io__state).
> :- mode browser2(in, di, uo) is det.
> browser2(State) -->

s/browser2/browser_2/g

> 	( { Command = quit } ->
> 		io__write_string("quitting...\n")
...
> :- pred startup_message(io__state::di, io__state::uo) is det.
> startup_message -->
> 	io__write_string("-- Simple Mercury Term Browser.\n"),
> 	io__write_string("-- Type \"help\" for help.\n\n").

The startup and quit messages will be very annoying if they
are printed every time you want to browse a term from within
the debugger.

> :- pred prompt(io__state::di, io__state::uo) is det.
> prompt -->
> 	io__write_string("browser> ").
> 
> 
> :- pred run_command(command, browser_state, browser_state,
> 	io__state, io__state).
> :- mode run_command(in, in, out, di, uo) is det.
> run_command(Command, State, NewState) -->
> 	( { Command = unknown } ->
> 		io__write_string("error: unknown command. try \"help\"\n"),

Sentences in error messages should start with capital letters
and end with full stops.

> :- pred portray(browser_state, io__state, io__state).
> :- mode portray(in, di, uo) is det.
> portray(State) -->
> 	{ get_fmt(State, Fmt) },
> 	( { Fmt = flat } ->
> 		portray_flat(State)
> 	; { Fmt = pretty } ->
> 		portray_pretty(State)
> 	; { Fmt = verbose } ->
> 		portray_verbose(State)
> 	;
> 		{ error("portray: domain error") }
> 	).

You should use a switch (i.e. a disjunction) rather than using an
if-then-else chain with `error' in the else case.

> :- pred portray_fmt(browser_state, portray_format, io__state, io__state).
> :- mode portray_fmt(in, in, di, uo) is det.
> portray_fmt(State, Format) -->
> 	( { Format = flat } ->
> 		portray_flat(State)
> 	; { Format = pretty } ->
> 		portray_pretty(State)
> 	; { Format = verbose } ->
> 		portray_verbose(State)
> 	;
> 		{ error("portray_fmt: domain error") }
> 	).

Likewise.

> 	% Single-line representation of a term.
> 
> :- pred term_to_string(univ, int, int, string).
> :- mode term_to_string(in, in, in, out) is det.
> term_to_string(Univ, MaxSize, MaxDepth, Str) :-
> 	CurSize = 0,
> 	CurDepth = 0,
> 	term_to_string2(Univ, MaxSize, CurSize, _NewSize,
> 		MaxDepth, CurDepth, Str).
> 
> 	% Note: When the size limit is reached, we simply display
> 	% further subterms compressed. We don't just stopping printing.
> 	% XXX: Is this reasonable?
> :- pred term_to_string2(univ, int, int, int, int, int, string).
> :- mode term_to_string2(in, in, in, out, in, in, out) is det.
> term_to_string2(Univ, MaxSize, CurSize, NewSize, MaxDepth, CurDepth, Str) :-

s/string/string_2/g

Likewise elsewhere.  This is just for consistency with other
source code in the Mercury implementation.

> :- pred comma_args(list(string), string).
> :- mode comma_args(in, out) is det.
> comma_args(Args, Str) :-
> 	( Args = [] ->
> 		Str = ""
> 	; Args = [S] ->
> 		Str = S
> 	; Args = [S1,S2|Ss] ->
> 		comma_args([S2|Ss], Rest),

Please put spaces after commas and around `|' operators.

> write_path([Dir,Dir2|Dirs]) -->
> 	write_path2([Dir,Dir2|Dirs]).

Ditto.

> :- pred default_state(univ, browser_state).
> :- mode default_state(in, out) is det.
> default_state(Univ, State) :-
> 	State = browser_state(Univ,3,10,[],verbose, 79, 25).

Ditto.

> :- pred set_term(univ, browser_state, browser_state).
> :- mode set_term(in, in, out) is det.
> set_term(NewUniv, browser_state(_OldUniv,Dep,Siz,Path,Fmt,X,Y),
> 	browser_state(NewUniv,Dep,Siz,Path,Fmt,X,Y)).

Ditto.

> :- pred show_settings(browser_state, io__state, io__state).
> :- mode show_settings(in, di, uo) is det.
> show_settings(State) -->
...
> 	io__write_string("Print format is "),
> 		io__write_string(FmtStr), io__nl,
> 	( { Fmt = flat } ->
> 		{ FmtStr = "flat" }
> 	; { Fmt = pretty } ->
> 		{ FmtStr = "pretty" }
> 	; { Fmt = verbose } ->
> 		{ FmtStr = "verbose" }
> 	;
> 		{ error("show_settings: domain error") }
> 	).

Use a switch rather than an if-then-else.
Or more simply, just change that to

	io__write_string("Print format is "),
 		io__print(FmtStr), io__nl,

> :- pred simplify(list(dir), list(dir)).
> :- mode simplify(in, out) is det.
> simplify([], []).
> simplify([parent|Dirs], Dirs).
> simplify([child(Dir)], [child(Dir)]).
> simplify([child(_Dir), parent |Dirs], Dirs).
> simplify([child(Dir1), child(Dir2) |Dirs], [child(Dir1) | Rest]) :-
> 	simplify([child(Dir2)|Dirs], Rest).

Please put spaces around `|' operators.

I didn't understand what this predicate is supposed to be doing --
a comment would help.

parse.m:
> %	path:
> %		["/"] dir ["/" path]

Hmm, that grammar allows "1//2" as a valid path, probably it shouldn't.

> 	% XXX: could nuke zero-arity "ls" and "cd".
> :- type command
> 	--->	ls(path)
> 	;	ls
> 	;	cd(path)
> 	;	cd

"XXX" is supposed to indicate something that should be done,
rather than merely something that could be done.

> 	% XXX: default depth also in browse.m
> :- pred default_depth(int).
> :- mode default_depth(out) is det.
> default_depth(10).

I think you should fix that one.

> :- pred parse_path(list(token), path).
> :- mode parse_path(in, out) is semidet.
> 	% SICStus is forgiving in the syntax of paths, hence so are we.
> 	% XXX: Be less forgiving?

Yes, I think it would better to be less forgiving.
If the user types something syntactically incorrect,
then it's probably more helpful to print an error message
than to ignore the error.

> :- pred parse_setting(list(token), setting).
> :- mode parse_setting(in, out) is semidet.
> parse_setting([Tok|Toks], Setting) :-
> 	( Tok = name("depth") ->
> 		Toks = [num(Depth)],
> 		Setting = depth(Depth)
> 	; Tok = name("size") ->
> 		Toks = [num(Size)],
> 		Setting = size(Size)
> 	; Tok = name("clipx") ->
> 		Toks = [num(X)],
> 		Setting = clipx(X)
> 	; Tok = name("clipy") ->
> 		Toks = [num(Y)],
> 		Setting = clipy(Y)
> 	; Tok = name("format") ->
> 		Toks = [Fmt],
> 		( Fmt = name("flat") ->
> 			Setting = format(flat)
> 		; Fmt = name("pretty") ->
> 			Setting = format(pretty)
> 		;
> 			Fmt = name("verbose"),
> 			Setting = format(verbose)
> 		)
> 	;
> 		fail
> 	).

It would be more efficient to use switches rather than if-then-elses.

util.m:
> :- module util.
> 
> :- interface.
> 
> :- import_module std_util, string, list.
> 
> :- pred util__typename(univ, string).
> :- mode util__typename(in, out) is det.
> 
> :- pred util__functor(univ, string).
> :- mode util__functor(in, out) is det.
> 
> :- pred util__args(univ, list(univ)).
> :- mode util__args(in, out) is det.
> 
> :- pred util__arity(univ, int).
> :- mode util__arity(in, out) is det.
> 
> :- pred util__copy(int, T, list(T)).
> :- mode util__copy(in, in, out) is det.
> 
> :- pred util__zip_with(pred(T1, T2, T3), list(T1), list(T2), list(T3)).
> :- mode util__zip_with(pred(in, in, out) is det, in, in, out) is det.
> 
> :- pred util__limit(pred(list(T), list(T)), list(T), list(T)).
> :- mode util__limit(pred(in,out) is det, in, out) is det.
> 
> :- implementation.
> 
> :- import_module std_util, list, string, int, require.
> 
> 
> util__functor(Univ, Functor) :-
> 	deconstruct(Univ, Functor, _Arity, _Args).
> 	
> util__args(Univ, Args) :-
> 	deconstruct(Univ, _Functor, _Arity, Args).
> 
> util__arity(Univ, Arity) :-
> 	deconstruct(Univ, _Functor, Arity, _Args).

It might be more efficient to use std_util__functor
to implement util__functor and util__arity.

But this layer of abstraction adds a small amount of
additional complexity -- what does it buy you in return?
Why not just use std_util__deconstruct or std_util__functor directly? 

> util__typename(Univ, TypeName) :-
> 	TypeInfo = univ_type(Univ),
> 	TypeName = type_name(TypeInfo).

This too seems hardly worth a subroutine -- you could just write

	TypeName = type_name(univ_type(Univ))

instead of

	util__typename(Univ, TypeName).

> util__copy(N, X, Xs) :-
> 	( N =< 0 ->
> 		Xs = []
> 	;
> 		util__copy(N1, X, Xs2),
> 		Xs = [X|Xs2],
> 		N1 is N - 1
> 	).

I think this does the same thing as list__duplicate, 
so you should use list__duplicate instead.

> New file: frame.m
> 	% XXX: Make frame type abstract instead?
> 	% XXX: I'd rather not fully module-qualify the frame type.
> % :- type frame.
> :- type frame == list(string).

I don't understand the second XXX comment.  Could you elaborate, please?

> :- pred frame__hsize(frame, int).
> :- mode frame__hsize(in, out) is det.
> 
> :- pred frame__vsize(frame, int).
> :- mode frame__vsize(in, out) is det.
> 
> :- pred frame__from_string(string, frame).
> :- mode frame__from_string(in, out) is det.
> 
> :- pred frame__vglue(frame, frame, frame).
> :- mode frame__vglue(in, in, out) is det.
> 
> :- pred frame__hglue(frame, frame, frame).
> :- mode frame__hglue(in, in, out) is det.
> 
> :- pred frame__clip(frame__clip_rect, frame, frame).
> :- mode frame__clip(in, in, out) is det.

These predicates should be documented in the interface section.

> 	% Clip a frame to the rectangle ((0,0),(X,Y)) where
> 	% origin is on the top-left. Coordinate axes go down and right.
> 	% XXX: Any reasons to implement a general clip?
> frame__clip(X-Y, Frame, ClippedFrame) :-
> 	list__take_upto(Y, Frame, YClippedFrame),
> 	list__map(left(X), YClippedFrame, ClippedFrame).

The "XXX" comment here isn't clear to me.  Could you please elaborate?

--------------------

Apart from that, it looks OK to me.  But could you please post another diff
and/or a relative diff after you've addressed those changes?

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