[m-rev.] for review: Java implementation of string library

[Fergus Henderson]

On 12-Dec-2003, James Goddard <goddardjames at yahoo.com> wrote:
> 
> Implemented some library functions for the string library in java.
...
> Index: string.m
> +:- pragma foreign_proc("Java",
> +	string__to_float(FloatString::in, FloatVal::out),
> +	[will_not_call_mercury, promise_pure, thread_safe],
> +"
> +	FloatVal = 0.0;		// FloatVal must be initialized to suppress
> +				// error messages when the predicate fails.
> +
> +	// leading or trailing whitespace is not allowed
> +	if (FloatString.length() == 0 || FloatString.trim() != FloatString)
> +	{

The "{" should be on the same line as the "if".

> +		} catch(java.lang.NumberFormatException e) {
> +			if (""nan"".compareTo(FloatString.toLowerCase()) == 0)
> +			{
> +				FloatVal = java.lang.Double.NaN;
> +				succeeded = true;
> +			} else if (""infinity"".compareTo(
> +					FloatString.toLowerCase()) == 0)
> +			{
> +				FloatVal = java.lang.Double.POSITIVE_INFINITY;
> +				succeeded = true;
> +			} else if (""infinity"".compareTo(
> +					FloatString.toLowerCase().substring(1))
> +					== 0)
> +			{

It would be simpler to use the equals() method rather than using
compareTo(...) == 0.

Also, this code is not as efficient as it could be; it calls toLowerCase()
three times, each of which will make a copy of the string, which
will increase the frequency of garbage collection.  Please use the
equalsIgnoreCase() method.  The last case there, where you are also
calling substring(1), can be done as

			} else if (FloatString.substring(1).
					equalsIgnoreCase(""infinity""))
			{

> @@ -3137,6 +3222,12 @@
>  		System.Convert.ToString(Ch), 
>  		Str0.Substring(Index + 1));
>  ").
> +:- pragma foreign_proc("Java",
> +	string__unsafe_set_char(Ch::in, Index::in, Str0::in, Str::out),
> +	[will_not_call_mercury, promise_pure, thread_safe],
> +"
> +	Str = Str0.substring(0,Index) + Ch + Str0.substring(Index+1);
> +").

s/,/, /
s/+/ + /

> +:- pragma foreign_proc("Java",
> +	string__unsafe_set_char(Ch::in, Index::in, Str0::di, Str::uo),
> +	[will_not_call_mercury, promise_pure, thread_safe],
> +"
> +	Str = Str0 = Str0.substring(0,Index) + Ch + Str0.substring(Index+1);
> +").

Likewise.  Also s/Str0 = //

> +:- pragma foreign_proc("Java",
> +	string__unsafe_substring(Str::in, Start::in, Count::in, SubString::uo),
> +	[will_not_call_mercury, promise_pure, thread_safe],
> +"
> +	SubString = Str.substring(Start, Start+Count+1);
> +").

s/+/ + / 
(twice)

Otherwise, that looks fine.  Please post a new diff (or relative diff)
when you have addressed those comments.

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