[Mono-list] Array.cs

[Paolo Molaro]

On 03/06/02 Martin Baulig wrote:
> > >     I'm also planning to implement this directly in C so that it can use
> > >     memcpy() whenever possible.
> > 
> > See icall.c:ves_icall_System_Array_FastCopy().
> 
> Well, this internall is both unused and incomplete. It needs to check
> whether both arrays are of the same type and use the "normal" copy if not.

Well, the fact that it doesn't do what you expect is more a problem of
lacking documentation than incompleteness. It's currently not used
because after I did some other changes to reflection, the array copy
didn't show up in my profile for slow methods and I had other priorities
than speed it up, so I left it in the source, but didn't activate its
use. FastCopy() is supposed to do a copy from one array to another _when
the arguments have already been checked_. It does that nicely and fast
(though it hasn't been tested much and I just found a bug in it:-).

The topic already showed up for String and other classes: the checks
and exception throwing should be done as much as possible in C# code.
So I don't agree with your change of removing the Copy implementation
and doing all the checks in the internalcall. Please, don't commit that
patch: just do the argument checking in C# and then call the fast copy
internalcall.

> > Several Array test fail for me here after updating to cvs (though they
> > likely failed before as well).
> 
> Can you give me a list of which tests fail ?

These are the tests that failed:
1) TestRank(MonoTests.System.ArrayTest): Value does not fall within the
expected range.(System.ArgumentException)
   at
System.Runtime.CompilerServices.RuntimeHelpers.InitializeArray(Array
array, RuntimeFieldHandle fldHandle)
   at MonoTests.System.ArrayTest.TestRank() in
d:\mcs\class\corlib\Test\System\ArrayTest.cs:line 66
2) TestBinarySearch1(MonoTests.System.ArrayTest): Value cannot be null.
Parameter name: array(System.ArgumentNullException)
   at System.Array.BinarySearch(Array array, Object value)
   at MonoTests.System.ArrayTest.TestBinarySearch1() in
d:\mcs\class\corlib\Test\System\ArrayTest.cs:line 79
3) TestBinarySearch2(MonoTests.System.ArrayTest): Value cannot be null.
Parameter name: array(System.ArgumentNullException)
   at System.Array.BinarySearch(Array array, Int32 index, Int32 length,
Object value, IComparer comparer)
   at System.Array.BinarySearch(Array array, Int32 index, Int32 length,
Object value)
   at MonoTests.System.ArrayTest.TestBinarySearch2() in
d:\mcs\class\corlib\Test\System\ArrayTest.cs:line 122
4) TestClear(MonoTests.System.ArrayTest): Array cannot be null.
Parameter name: array(System.ArgumentNullException)
   at System.Array.Clear(Array array, Int32 index, Int32 length)
   at MonoTests.System.ArrayTest.TestClear() in
d:\mcs\class\corlib\Test\System\ArrayTest.cs:line 194
[...]
Many other array tests failing here...
[...]
29) TestSort(MonoTests.System.ArrayTest): Array cannot be null.
Parameter name: array(System.ArgumentNullException)
   at System.Array.Sort(Array array)
   at MonoTests.System.ArrayTest.TestSort() in
d:\mcs\class\corlib\Test\System\ArrayTest.cs:line 1631

I haven't rerun the tests since then.

> Well, you just found a bug in the documentation. Your code is
> incorrect according to the documentation (it says the the exceptio is
> thrown if either "index is equal to or greater than the length of
> array" or "the number of element in the source array is greater than
> the available space from index to the end of the destination array"),
> but it works fine with the microsoft runtime.
> 
> So according to the docu, both checks must be done - but according to
> the implementation, only your check is to be done.

It didn't happen by chance:-) I hit the bug, read the docs, determined that
the first check didn't make much sense with length==0, created the test 
program and confirmed the behaviour on windows and changed our implementation 
to match.

lupus

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