[m-rev.] for review: stack -lN and nondet_stack -lN

[Peter Moulder]

For comparison, the corresponding gdb behaviour is a simple number
argument (no flag) that may be positive (innermost N frames) or negative
(outermost N frames).  In the positive case, the "snipped" message is
"(More stack frames follow...)\n".

I say don't bother allowing for any future mechanism to limit output to
N lines: use the normal paging mechanism, and the user and press `q'
after the first page if they want.  They can even temporarily set the
screen height shorter if they really want.

(I haven't checked that mdb already uses paging for stack output, but if
it isn't then I think it better to add paging and no "limit to N lines"
feature.  gdb seems to implement paging by using fputs_filtered etc.
functions defined in gdb/utils.c.)

Without thinking too much, my inclination is to drop the `-l' and just
take an optional numerical argument like step etc. and like gdb's stack
(backtrace/where/info stack) command.  This makes things easier for
people moving between gdb and mdb.


The documentation says "nonzero", but the code (in
MR_dump_nondet_stack_from_layout) says "limit > 0".  I
think the documentation should be changed, just in case someone later
implements the gdb feature of outermost N (-N) frames.  ("nonzero"
appears several times in the patch, btw.)

Around the sscanf in
trace/mercury_trace_internal.c:MR_trace_options_stack_trace, there's no
diagnosis of negative argument.

On a trivial note, I suggest s/cat/category/ for the argument name to
that function.  (I looked back to the caller to work out what cat
meant.)  (Ah, actually, I see that all the other functions already use
cat for the same thing, so perhaps don't bother.)

Currently "-l5x" will be parsed identically to "-l5", with no error
message.  You might add "/* XXX: use strtol so that trailing garbage is
diagnosed. */".

Oh well, modifying some code from something I wrote elsewhere:

	{
		char *endptr;
		long long_nframes;
		errno = 0;
		long_nframes = strtol(MR_optarg, &endptr, 10);
		if ((*MR_optarg == '\0') || (*endptr != '\0')	/* not an integer */
		    || (errno != 0) || (long_nframes > INT_MAX) || (long_nframes < 0))	/* out of range */
		{
			MR_trace_usage(cat, item);
			return MR_FALSE;
		}
		*limit = long_nframes;
	}


Otherwise the patch looks fine that I can see.

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