[m-rev.] for review: mdb command line completion

[Fergus Henderson]

On 04-Mar-2002, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> 
> Add support for command line completion to mdb.

One thing I noticed is that there are no test cases.

I know we have had some issues with different versions of readline
behaving differently, but even if we can't easily get the tests to work
on all our machines, I think it is worth setting up some tests on at
least one machine.

One way to make it testable, without affecting any of the
existing debugger test cases, would be to add a new runtime option
`--force-readline', which would cause MR_trace_readline() to use
readline() even if the input file was a tty.

> runtime/mercury_array_macros.h:
> 	Define a macro MR_find_first_match, which is like MR_bsearch
> 	except that it finds the first match, not an arbitary match.

s/arbitary/arbitrary/

> Index: runtime/mercury_array_macros.h
>  /*
> +** MR_find_first_match(int next, int& element, MR_bool& found, COMPARE)
> +**
> +** Given a sorted array, this macro finds the first element in the array
> +** for which `COMPARE' is zero (MR_bsearch finds an arbitrary element).
> +** Otherwise, the parameters and behaviour are the same as for MR_bsearch.

The parameter `next' is poorly named.
I suggest s/next/num_elements/
(both here and in MR_bsearch()).

> +#define MR_find_first_match(next, element, found, COMPARE)		\
> +	do {								\
> +		MR_bsearch((next), (element), (found), (COMPARE));	\
> +		if (found) {						\
> +			while ((element) > 0) {				\
> +				int	diff;				\

`diff' is unused here.
That line should be deleted.

> Index: trace/mercury_trace_alias.c
...
> +static MR_bool
> +MR_trace_filter_alias_completions(const char *word, MR_Completer_Data *data)

Why not `const MR_Completer_Data *data'?

> Index: trace/mercury_trace_completion.c
...
> +#define MR_MAX_COMMAND_LEN 32

You should document
	- what this is used for
	- what the units are

Also, 32 is probably too small.

> +typedef struct MR_String_Array_Completer_Data_struct {
> +	int offset;
> +	char **strings;
> +} MR_String_Array_Completer_Data;

What does the `offset' field represent?
Some comments here might help.

> +char *MR_trace_line_completer(const char *passed_word, int state)
> +{
> +#ifndef MR_NO_USE_READLINE
... 150 lines of code ...
> +#else
> +    return NULL;
> +#endif

You should add comments after the #else and #endif.

It would IMHO also be clearer to use `#ifdef' rather than `#ifndef' here,
swapping the `then' and the `else' parts.  Then you won't need a comment
on the #else.

> +++ trace/mercury_trace_internal.c	4 Mar 2002 07:25:14 -0000
> +		/*
> +		** Some commands take set strings as arguments.
> +		** This field is a NULL terminated array of those strings.
> +		*/
> +	const char *const	*MR_trace_command_arg_strings;

What is this field set to for commands which don't take strings as arguments?
The comment should document that.

> +static const char *const	MR_trace_print_cmd_args[] =
> +		{"exception", "goal", NULL};

For use with filename completion, it might be worth making
"print variables" have the same effect as "print".

> +	** XXX Should we allow completion of options of comands?

s/comands/commands/

The answer to the XXX question is yes ;-)
So I'd write that as

	** XXX We should allow completion of options of commands.

> +	{ "queries", "query", NULL, MR_trace_null_completer },
> +	{ "queries", "cc_query", NULL, MR_trace_null_completer },
> +	{ "queries", "io_query", NULL, MR_trace_null_completer },

These should be using a module-name completer.
An XXX comment is warranted.

(Ideally the completer would be one that included all the modules
in the program, not just those compiled with tracing enabled, so
maybe this is not the same completer as for the `procedures' command.)

> Index: trace/mercury_trace_tables.c
> +/*
> +** Convert all occurrences of `__' to `:'.

Since we're planning to eventually change the module qualifier
from `:' to `.', it might be a good idea to use a macro
MR_MODULE_SEPARATOR_CHAR instead of the various different
hard-coded occurrences of `:'.

> +*/
> +static char *
> +MR_translate_double_underscores(char *start)
...
> +	return start;
> +}

The return type for that should be `void *'.

(Whoever designed C's string handling routines has a lot to answer for ;-)

Apart from that, 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
--------------------------------------------------------------------------