[Mono-devel-list] [Patch] First patch for making String managed

[Paolo Molaro]

On 06/01/04 Andreas Nahr wrote:
> Index: String.cs
> ===================================================================
> RCS file: /cvs/public/mcs/class/corlib/System/String.cs,v
> retrieving revision 1.113
> diff -u -r1.113 String.cs
> --- String.cs	25 May 2004 17:10:39 -0000	1.113
> +++ String.cs	1 Jun 2004 13:09:26 -0000
> @@ -407,25 +407,49 @@
>  			return (0 == Compare (this, length - value.length, value, 0, value.length));
>  		}
>  
> -		public int IndexOfAny (char [] anyOf)
> +		public unsafe int IndexOfAny (char [] anyOf)
>  		{
>  			if (anyOf == null)
>  				throw new ArgumentNullException ("anyOf");
>  
> -			return InternalIndexOfAny (anyOf, 0, this.length);
> +			fixed (char* test = &start_char, testChars = anyOf) {
> +				char* testPtr = test;
> +				char* testChar = testChars, targetChar = testChar + anyOf.Length;
> +				for (int pos = 0; pos < this.length; pos++) {
> +					for (; testChar < targetChar;) {
> +						if (*testPtr == *testChar)
> +							return pos;
> +						testChar++;
> +					}
> +					testPtr++;
> +				}
> +			}

testChar should be reset each time the inner loop is entered first.
Also, I think you'll get an exception if this implementation is called
on an empty anyOf. Bonus points in the next patch submissing if test
cases for those issues are added to the test suite.
I'm not opposed to 'some' code duplication for the sake of speed, but
only when the code has been extensively tested. If we have performance
issues when inlining we need to fix them anyway. So please just change
the implementation of the most specific method (with start and count):
after it has been tested and if inlining doesn't work in that case and
the performance difference is significant, we'll duplicate the code.

> +		public unsafe int LastIndexOfAny (char [] anyOf)
>  		{
>  			if (anyOf == null)
>  				throw new ArgumentNullException ("anyOf");
>  
> -			return InternalLastIndexOfAny (anyOf, this.length - 1, this.length);
> +			fixed (char* test = &start_char, testChars = anyOf) {
> +				char* testPtr = test + this.length;
> +				char* testChar = testChars, targetChar = testChar + anyOf.Length;
> +				for (int pos = this.length; pos > 0; pos--) {
> +					for (; testChar < targetChar;) {
> +						if (*testPtr == *testChar)
> +							return pos;
> +						testChar++;
> +					}
> +					testPtr--;
> +				}

Similar issue as above, plus one new: this code will access the null
char after the string data: while it is harmless, it's still a bug since
it can return this.length if anyOf contains '\0'.
The loop needs to start from this.length - 1.

> -		public int LastIndexOf (char value, int startIndex)
> +		public unsafe int LastIndexOf (char value, int startIndex)
>  		{
> -			return LastIndexOf (value, startIndex, startIndex + 1);
> +			fixed (char* test = &start_char) {
> +				char* testPtr = test + startIndex;

This removes the check that was in the called LastIndexOf () method for
a safe startIndex: you allow for random memory to be read.

There may be other bugs, I didn't check all the code: please prepare a
patch with no duplicated code first so the code can be better reviewed
and tested.
Thanks.

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better