[Mono-dev] [PATCH] Fix Type.Equals to support user defined types (required by vbnc)

[Paolo Molaro]

On 10/29/06 Kornél Pál wrote:
> I forgot to attach the diff file.:)

You also forgot the nunit tests that are required to be able to commit
a change so deep in the runtime a few days before a major release:)
Also, please don't post patches marked as octect-stream, they are text
files, post them as such.

> Index: mcs/class/corlib/System/Type.cs
> ===================================================================
> --- mcs/class/corlib/System/Type.cs	(revision 67080)
> +++ mcs/class/corlib/System/Type.cs	(working copy)
> @@ -411,20 +411,26 @@
>  
>  		public override bool Equals (object o)
>  		{
> +			return Equals (o as Type);
> +		}
> +
> +		[MethodImplAttribute (MethodImplOptions.InternalCall)]
> +		internal static extern bool type_equals (Type type1, Type type2);
> +
> +		public bool Equals (Type o)
> +		{
>  			if (o == null)
>  				return false;
> -			
> -			// TODO: return UnderlyingSystemType == o.UnderlyingSystemType;
> -			Type cmp = o as Type;
> -			if (cmp == null)
> -				return false;
> -			return Equals (cmp);
> +
> +			// User defined types depend on this behavior
> +			Type type1 = UnderlyingSystemType;
> +			Type type2 = o.UnderlyingSystemType;

Can two user-defined types that are reference-equal not match in Equals()?
It doesn't look possible, so the fast check should be placed first.

> +			return type1 == type2 ||
> +				(type1 != null && type2 != null && type1.IsSystemType && type2.IsSystemType && type_equals (type1, type2));

It looks like you're optimizing for the case of user-defined types, by
moving several checks from the icall to here where the common case is to
perform the icall in any case. The rule to follow, instead, is:
	never speedup the usage of user-defined types if it means
	slowing down the types that every sane person uses

A 1% speedup of common types is worth a 10x slowdown of user-defined
types.

>  		}
>  
>  		[MethodImplAttribute(MethodImplOptions.InternalCall)]
> -		public extern bool Equals (Type type);
> -		
> -		[MethodImplAttribute(MethodImplOptions.InternalCall)]
>  		private static extern Type internal_from_handle (IntPtr handle);
>  		
>  		[MethodImplAttribute(MethodImplOptions.InternalCall)]
> @@ -690,7 +696,13 @@
>  		
>  		public override int GetHashCode()
>  		{
> -			return (int)_impl.Value;
> +			// User defined types depend on this behavior

For every such comment you put in the sources we need at least a nunit test.

> +			Type type = UnderlyingSystemType;
> +
> +			if (type != this)
> +				return type.GetHashCode ();

In the above code you check for UnderlyingSystemType being null:
if it's the case here, too, you'll cause a nullref exception.

> +
> +			return base.GetHashCode ();
>  		}
>  
>  		public MemberInfo[] GetMember (string name)
> Index: mcs/class/corlib/System/MonoType.cs
> ===================================================================
> --- mcs/class/corlib/System/MonoType.cs	(revision 67080)
> +++ mcs/class/corlib/System/MonoType.cs	(working copy)
> @@ -565,8 +565,23 @@
>  			UnitySerializationHolder.GetTypeData (this, info, context);
>  		}
>  
> -		public override string ToString()
> +		public override bool Equals (object o)
>  		{
> +			if (o == this)
> +				return true;
> +
> +			Type type = o as Type;
> +
> +			return type != null && type.IsSystemType && type_equals (this, type);

Same comment about optimizing for the user-defined types.

> +		}
> +
> +		public override int GetHashCode ()
> +		{
> +			return (int) _impl.Value;
> +		}
> +
> +		public override string ToString ()
> +		{
>  			return getFullName (false, false);
>  		}
>  
> Index: mono/mono/metadata/icall-def.h
> ===================================================================
> --- mono/mono/metadata/icall-def.h	(revision 67080)
> +++ mono/mono/metadata/icall-def.h	(working copy)
> @@ -797,26 +797,26 @@
>  ICALL(WAITH_3, "WaitOne_internal", ves_icall_System_Threading_WaitHandle_WaitOne_internal)
>  
>  ICALL_TYPE(TYPE, "System.Type", TYPE_1)
> -ICALL(TYPE_1, "Equals", ves_icall_type_Equals)

As the comment at the start of this file says, do not do useless renames
of all the entries, there is no larger diff competition going on.
Besides you can just name the method EqualsInternal and have the
enums still ordered (which doesn't mean anything, the names can be
anything).
Thanks.

lupus

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