#884 [csharp] getCPtr should use object.ReferenceEquals

[Koro]

The internal getCPtr function uses operator== to compare
the "obj" parameters instead of using object.ReferenceEquals. This has bad side effects if the class has defined operator== (with %typemap(cscode)) that calls another wrapped function in the class (leading to an infinite loop)

As an attachment, a patch to correct the problem (which I hope I created correctly).

[Koro]

Fix

[William Fulton]

Using (object)obj == null could also be used instead. In the MSDN docs http://msdn.microsoft.com/en-us/library/ms173147\(VS.80).aspx on overloading operator==, it suggests the operator== should be using the cast to object. Perhaps your operator== isn't following the guidelines? Can you please provide a testcase showing the problem? Thanks.

[Koro]

This was from a while ago, at a job I don't even work at anymore :)

Basically I was trying to work around the fact that the csharp module doesn't wrap C++ operators. So what I did basically was:

1- %rename all of C++ operators in a class with "proper names" (aka operator== becomes OperatorEqual) so they could be wrapped as regular functions.
2- Make said functions private in the C# wrapper class.
3- Use %typemap(cscode) to add code to the C# class implementing a proper operator== that just calls the private function from 1.

Now obviously, OperatorEqual will call getCPtr, which uses == (aka the C# class's operator==) to check if obj is null. Since I defined it to call OperatorEqual, which internaly calls getCPtr... bam, infinite recursion.

I don't think it matters much if Object.ReferenceEquals or (object)obj==null is used to compare if obj is null, as long as it makes sure to not call the operator== function if it's defined.

[Olly Betts]

The test in git master is still (obj == null).

[Olly Betts]

It seems like the case which triggered this report was probably really due to incorrect code elsewhere.

However it seems to me that when SWIG is checking if the object is null in such cases it would still be better for it do (object)obj == null (and similarly for some obj != null tests I see which the patch here doesn't address) as it really does want an exact null check and it isn't necessary to be calling an overloaded == in this case. Usually what SWIG currently does just incurs a small but avoidable runtime overhead, but it could be a problem if the overloaded == says some things are equal to null which aren't themselves null (or does C# say that only null should compare equal to null?)

@wfulton Thoughts? It seems this ticket could be fairly easily resolved by either making a change or saying it's just user error.

[Olly Betts]

https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/statements-expressions-operators/how-to-define-value-equality-for-a-type seems to be the current version of the link wsfulton gave above.

[William Fulton]

I'm not sure it will be easy to keep the code based maintained to always cover this corner case. The relevant typemaps can be easily changed by the user to use ReferenceEquals instead and so I'm closing as won't change. I can't there being a runtime overhead when there is no operator== overload.