[m-dev.] For review: improvements to string.m

[Fergus Henderson]

On 22-Sep-2000, Ralph Becket <rbeck at microsoft.com> wrote:
> I've taken Fergus' comments on board.  Here's the latest log & diff:
> 
> Estimated hours taken: 0.5
> 
> Fixed some inefficiencies in the string module and a small bug.
...
> 	string__index/3 in its (in, in, out) mode did not check that
> 	the index was non- negative; it does now.

>  :- pragma c_code(string__index(Str::in, Index::in, Ch::out),
>  		[will_not_call_mercury, thread_safe], "
> -	if ((MR_Word) Index >= strlen(Str)) {
> +	if (0 > (MR_Word) Index || (MR_Word) Index >= strlen(Str)) {

The fix is wrong -- MR_Word is an unsigned type,
and so the test `0 > (MR_Word) Index' will never succeed.

The test `(MR_Word) Index >= strlen(Str)' will catch all cases
where Index < 0, except for those where strlen(Str) > MAXINT.
The documentation in library/int.m says that the behaviour in
cases where integer arithmetic overflows is undefined.
So I don't think the spec requires us to handle those cases.
>From a practical perspective, I don't think it's worth taking
even a small efficiency penalty to handle those cases correctly, since
(a) for such cases you'll get integer overflow, so probably something
will break somewhere anyway, even if string__index happens to get it
right and (b) such cases don't occur in practice -- you basically
never use half your address space for a single string (among other
reasons, it would probably thrash most systems into the ground).
If you're using half of your memory for a single string, then
string__index is likely to be the least of your problems.

So I don't think you should commit that change.

An alternative would be to add some documentation
explaining why we don't bother checking that case.

Apart from that, the remainder of your change looked fine.
The new string functions should be mentioned in the NEWS file.

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