[m-rev.] for review: string__join_list

[Fergus Henderson]

On 25-Jun-2001, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> For review by anyone. No test case is necessary, since my next change to the
> deep profiler will use the new predicate.

A test case would still be a good idea, since the deep profiler will normally
only be built in the default grade, whereas we build and run the test cases in
lots of different grades.  The deep profiler wouldn't show up any bugs in the
implementation of this routine for e.g. the IL back-end or the Java back-end.

> library/string.m:
> 	Add a function string__join_list(Separator, Strings), which is like
> 	string__append_list, except it puts Separator between adjacent strings
> 	in Strings.
...
> Index: string.m
> +	% Implementation of string__join_list that uses C as this
> +	% minimises the amount of garbage created.

Is this function really important enough to warrant optimizing it by
implementing it in C?

Implementing it in C definitely increases the risk of bugs... see below ;-)

> +:- pragma foreign_proc("C", string__join_list(Sep::in, Strs::in) = (Str::out),
>  		[will_not_call_mercury, thread_safe], "{
> +	MR_Word	list = Strs;
> +	MR_Word	tmp;
> +	size_t	len = 0;
> +	size_t	sep_len;
> +	bool	add_sep;
> +
> +	sep_len = strlen(Sep);
> +
> +		/* Determine the total len of all strings */
> +	len = -sep_len; /* compensate for no separator before first string */

Hmm, `len' has type `size_t', which is unsigned, and you're assigning
a negative value to it.  Not a bug as such, since C will implicitly
convert the value, but not very nice style...

> +	while (!MR_list_is_empty(list)) {
> +		len += sep_len + strlen((MR_String) MR_list_head(list));
> +		list = MR_list_tail(list);
> +	}
> +
> +		/* Allocate enough word aligned memory for the string */
> +	if (len <= 0) {
> +		len = 0;
> +	}

This `if' statement has no effect: since `len' is unsigned,
the condition can only succeed if len == 0, in which case the
body will have no effect.

> +	MR_allocate_aligned_string_msg(Str, len, MR_PROC_LABEL);

If the list was empty, this will now try to allocate `(size_t)-sep_len'
bytes of memory, which will no doubt fail.

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