C# code generated for comparison methods

[kwaclaw]

A Slice struct can be mapped to a C# struct or class, depending on the Slice directive used. For the comparison methods and equality operators, slice2cs generates the same code for classes and structs. However, this code is not optimal for either case, since it tries to satisfy both.

As an example let's use this Slice declaration:

struct A { string str; int num; };

From http://msdn.microsoft.com/library/de...pconEquals.asp
and http://msdn.microsoft.com/library/de...lsoperator.asp
the basic guidelines are these:

* For value types, whenever you override Equals (instance method), you
should also overload the equality operators, as value types do not have
a default implementation for them.

* For reference types, do not overload the equality operators, as there is
a default implementation which uses the Equals() method. See
http://msdn.microsoft.com/library/de...alitytopic.asp.

* You rarely ever need to re-implement the *static* Equals method, as its
default implementation uses the instance method. From
http://msdn.microsoft.com/library/de...qualsTopic.asp
it becomes clear that the implementation generated by slice2cs just
duplicates the default.

The generated code look like this, the comments are based on the guidelines above:

public override bool Equals(object __other)
{
    // for struct mapping,this will likely box "this", always returning false
    if(object.ReferenceEquals(this, __other))
    {
	return true;
    }
    
    // This will evaluate to true for any two subclasses of A;
    // this may be OK if is it  desired that two different instances
    // are equal even if only their A portion is equal and they
    // don't override Equals(). Seems unusual.
    if(!(__other is A))
    {
	return false;
    }
    
    // If we want to avoid multiple "(A)__other" casts below, we could
    // use A other = (A)__other, but for structs this may be inefficient
    // as it leads to a struct copy, whereas otherwise the compiler
    // may optimize away the struct instantiation.
    
    // Why not use string.Equals(this.str, ((A)__other).str)? 
    if(str == null)
    {
	if(((A)__other).str != null)
	{
	    return false;
	}
    }
    else
    {
	if(!(str.Equals(((A)__other).str)))
	{
	    return false;
	}
    }
    
    if(!(num.Equals(((A)__other).num)))
    {
	return false;
    }
    return true;
}

// redundant - as per guidelines
public static bool Equals(A __lhs, A __rhs)
{
    return object.ReferenceEquals(__lhs, null)
	       ? object.ReferenceEquals(__rhs, null)
	       : __lhs.Equals(__rhs);
}

// overloading equality operators is recommended for value types,
// but not for reference types, so we should have two kinds of code
public static bool operator==(A __lhs, A __rhs)
{
    return Equals(__lhs, __rhs);
}

public static bool operator!=(A __lhs, A __rhs)
{
    return !Equals(__lhs, __rhs);
}

For struct mapping:
===================

Here is how it could be done specifically for C# structs:
(simpler because structs cannot be null or derived from)

public override bool Equals(object __other)
{
    // we can use 'is' because there are no structs derived from A
    return __other is A && this == (A)__other;
}

// no null checks necessary - we have structs as arguments
public static bool operator==(A __lhs, A __rhs)
{
    return string.Equals(__lhs.str, __rhs.str) && __lhs.num.Equals(__rhs.num);
}

public static bool operator!=(A __lhs, A __rhs)
{
    return !(__lhs == __rhs);
}

For class mapping:
==================

No static Equals() implementation, no operator overloads.
This would be a simpler approach, as it also avoids repeated casts.

public override bool Equals(object __other)
{
    // we could use "if (!(__other is A))" depending on the
    // desired semantics - the ususal (MSDN) way is shown below.
    if (__other == null || GetType() != __other.GetType()) 
	return false;
    // avoid multiple casts, no value type copying here
    A other  = (A)__other;
    // note we are not using ==, since value types do not have a default implementation for ==
    return string.Equals(this.str, other.str) && num.Equals(other.num);
}

[michi]

Thanks for the feedback!

I'll have a look at this and streamline it.

Cheers,

Michi.

[michi]

Originally Posted by kwaclaw

* For value types, whenever you override Equals (instance method), you
should also overload the equality operators, as value types do not have
a default implementation for them.

Right. The current mapping does that.

* For reference types, do not overload the equality operators, as there is
a default implementation which uses the Equals() method. See
http://msdn.microsoft.com/library/de...alitytopic.asp.

I disagree. From the documentation:

The following guidelines are for implementing a reference type:

Consider overriding Equals on a reference type if the semantics of the type are based on the fact that the type represents some value(s).
Most reference types must not overload the equality operator, even if they override Equals. However, if you are implementing a reference type that is intended to have value semantics, such as a complex number type, you must override the equality operator.

Structs that are mapped to classes have value semantics, so overriding operator== and operator!= is necessary. (At least, that's how I interpret the words in the doc.)

* You rarely ever need to re-implement the *static* Equals method, as its
default implementation uses the instance method. From
http://msdn.microsoft.com/library/de...qualsTopic.asp
it becomes clear that the implementation generated by slice2cs just
duplicates the default.

You are right, thanks. I'll remove the generated static Equals for the next release.

The generated code look like this, the comments are based on the guidelines above:

public override bool Equals(object __other)
{
    // for struct mapping,this will likely box "this", always returning false
    if(object.ReferenceEquals(this, __other))
    {
	return true;
    }

I agree. For the struct mapping for structures, that code shouldn't be there. I've fixed this for the next release.

    // This will evaluate to true for any two subclasses of A;
    // this may be OK if is it  desired that two different instances
    // are equal even if only their A portion is equal and they
    // don't override Equals(). Seems unusual.
    if(!(__other is A))
    {
	return false;
    }

I followed the usual C++ semantics here. For C++, they have the advantage that they are statically type safe. In particular, in C++, it's not possible to pass an object of entirely the wrong type to the operator=() member function.

For C#, I guess the situation is different because the formal parameter type of the non-static Equals is System.Object. On the whole, I agree with you; it is probably overall safer to generate Equals as you suggested, making sure that the types of both operands match.

Here is how it could be done specifically for C# structs:
(simpler because structs cannot be null or derived from)

public override bool Equals(object __other)
{
    // we can use 'is' because there are no structs derived from A
    return __other is A && this == (A)__other;
}

// no null checks necessary - we have structs as arguments
public static bool operator==(A __lhs, A __rhs)
{
    return string.Equals(__lhs.str, __rhs.str) && __lhs.num.Equals(__rhs.num);
}

public static bool operator!=(A __lhs, A __rhs)
{
    return !(__lhs == __rhs);
}

No, that doesn't work. This code ends up in infinite recursion because Equals() calls itself.

Cheers,

Michi.

[kwaclaw]

Originally Posted by michi

Right. The current mapping does that.

I disagree. From the documentation:

Structs that are mapped to classes have value semantics, so overriding operator== and operator!= is necessary. (At least, that's how I interpret the words in the doc.)

I think the docs are somewhat in contradiction. If you read this MSDN page: http://msdn.microsoft.com/library/de...pconEquals.asp, specifically the comments and code for the Point class, you will see that no operators are overloaded even though the example is meant to specifically provide value semantics.

However, there is nothing better than trying it out yourself, and the result is that you are right. The default reference type implementation of equality operators apparently does not call the static Equals() method, only ReferenceEquals(). I have to go back now and check some of my code. :-(

However, now that you have to overload the equality operators anyway, I suggest doing it like it is done for structs, that is, put the actual comparison code into the equality operator implementation, not into Equals(). The reason is that Equals() is a virtual method and cannot (currently) be inlined. So I suggest something like this (similar to the code for structs):

public override bool Equals(object __other) 
{ 
  if (__other == null || GetType() != __other.GetType()) 
    return false; 
  return this == (A)__other;
}

public static bool operator==(A __lhs, A __rhs)
{
  if (object.ReferenceEquals(__lhs, __rhs))
    return true;
  if (object.ReferenceEquals(__lhs, null) || object.ReferenceEquals(__rhs, null))
    return false;

  // return __lhs.str == __rhs.str && __lhs.num == __rhs.num;
  return string.Equals(__lhs.str, __rhs.str) && int.Equals(__lhs.num, __rhs.num);
}

public static bool operator!=(A __lhs, A __rhs)
{
  return !(__lhs == __rhs);
}

Originally Posted by michi

No, that doesn't work. This code ends up in infinite recursion because Equals() calls itself.

Not if you look closely. The code for the operator overload calls the static string.Equals and virtual int.Equals, not the virtual A.Equals.
I did it that way for the general case, since value types do not have a default implementation for the equality operators. For our specific example - strings and ints - I would probably just use this code:

return __lhs.str ==  __rhs.str && __lhs.num ==  __rhs.num;

Regards,

Karl