Re: C# - generic wrappers for std::vector<T>
William S Fulton <wsf <at> fultondesigns.co.uk>
2009-04-01 21:46:35 GMT
David Piepgrass wrote:
>> I've made changes to implement IEnumerable<> rather than IList<> as
>> IList<> ultimately requires the C++ operator== to be available, which it
>> often isn't.
>
> You can already put SWIG wrappers in List<T> objects, so it's kind of disingenuous to imply IList<T> can't
be supported. And List<T> doesn't use operator==, which is a statically bound call and therefore not
callable from generic classes. Instead it uses EqualityComparer<T>.Default, I believe, which must use Equals().
>
> The most important features of IList<T> such as Add, Remove, Clear, and this[index] do not use an equality
test, so I strongly encourage you to reconsider.
But in order to implement an interface, all the methods must be
implemented. IList<T> requires the IndexOf and Contains methods, which
in turn require the C++ std::find, which in turn requires a C++
operator==. This is because we are using value equality, not reference
equality, see my comments at the end. Basically I don't won't to put in
dependencies that will result in non-compileable code most of the time.
Note that although, the wrappers don't implement IList<>, they do
implement most of the methods that IList<> requires. The ones omitted
are the ones that rely on equality.
>
> Besides, any SWIG user could (and IMO, should) override Equals(object) and operator==() as I have done.
My approach is something like this, except altered to be Compact Framework compatible:
>
> %typemap(csclassmodifiers) SWIGTYPE "public partial class"
> %typemap(csclassmodifiers) SWIGTYPE *, SWIGTYPE &, SWIGTYPE [], SWIGTYPE (CLASS::*) "public partial struct"
>
> ////////////////////////////////////////////////////////////////////////////
> // Implement Equals() and GetHashCode() on all wrapper classes. The same C++
> // object may be represented by multiple instances of the proxy class, so we
> // need to change these functions so that two different instances of the proxy
> // class that point to the same C++ object are considered equal and have the
> // same hash code. At the same time, improve code sharing by putting these
> // functions, getCPtr() and swigCPtr in a common base class.
> //
> // Put Equals() and GetHashCode() in a base class ("CppWrapper") so that they
> // can be replaced, if desired, in a derived class.
> %pragma(csharp) imclassimports=%{
> using System;
> using System.Runtime.InteropServices;
> using System.Drawing;
>
> public class CppWrapper
> {
> protected HandleRef swigCPtr;
>
> public CppWrapper(IntPtr cPtr)
> { swigCPtr = new HandleRef(this, cPtr); }
>
> protected internal static IntPtr getCPtr(CppWrapper obj) {
> return object.ReferenceEquals(obj, null) ? IntPtr.Zero : (IntPtr)obj.swigCPtr;
> }
> public override bool Equals(object other_) {
> CppWrapper other = other_ as CppWrapper;
> return getCPtr(other) == swigCPtr;
> }
This implementation of Equals needs modifying to meet the conditions as
stated in the Object.Equals documentation -
http://msdn.microsoft.com/en-us/library/bsc2ak47(VS.80).aspx and
"Guidelines for Overloading Equals() and Operator == (C# Programming
Guide)" - http://msdn.microsoft.com/en-us/library/ms173147(VS.80).aspx.
> public override int GetHashCode() {
> return swigCPtr.ToInt32();
> }
XOR high and low bits as suggested in the Object.GetHashCode docs would
be better -
http://msdn.microsoft.com/en-us/library/system.object.gethashcode(VS.80).aspx
(for 64 bit pointers).
> public static bool operator!=(CppWrapper a, CppWrapper b)
> { return getCPtr(a) != getCPtr(b); }
> public static bool operator==(CppWrapper a, CppWrapper b)
> { return getCPtr(a) == getCPtr(b); }
> }
> %}
> %typemap(csbase) SWIGTYPE "CppWrapper"
Having a single swigCPtr in the inheritance hierarchy is dangerous, see
comments below.
> %typemap(csbase) SWIGTYPE *, SWIGTYPE &, SWIGTYPE [], SWIGTYPE (CLASS::*) ""
> // SWIG gives this absurd warning for derived classes: "Warning for
> // DerivedClass proxy: Base CppWrapper ignored. Multiple inheritance is not
> // supported in C#." Workaround: suppress that warning universally
> %warnfilter(SWIGWARN_CSHARP_MULTIPLE_INHERITANCE);
Didn't we fix this by providing the 'replace' attribute - Bug #1794247?
If not, please lodge a new bug including a small use case. I've seen you
post your typemaps a few times which is great, but would prefer up to
date code if doing so again so as to not mislead future 'googlers'.
> // Normal wrappers (for defined types) use this code
> %typemap(csbody, noblock=1) SWIGTYPE
> { //
> protected bool swigCMemOwn;
> protected internal $csclassname(IntPtr cPtr, bool cMemoryOwn) : base(cPtr) {
> if (!(swigCMemOwn = cMemoryOwn))
> GC.SuppressFinalize(this);
> }
This is a neat performance trick I've not seen before which I thought
might be useful for the default typemaps. But after some pondering I
think it is too risky to put into the default SWIG typemaps as it is
possible that some users customisations will set swigCMemOwn to true
after the constructor is called. I'd suggest making swigCMemOwn
readonly to future proof your code with a compilation error... once
you've done that I think you'll find that you could even get rid of the
swigCMemOwn flag altogether.
> }
>
> // Wrappers for unknown types (e.g. SWIGTYPE_p_int) use this code
> %typemap(csbody, noblock=1) SWIGTYPE *, SWIGTYPE &, SWIGTYPE []
> { //
> protected HandleRef swigCPtr;
> internal $csclassname(IntPtr cPtr, bool futureUse) { swigCPtr = new HandleRef(this, cPtr); }
>
> internal static HandleRef getCPtr(CppWrapper obj) {
> return (obj == null) ? IntPtr.Zero : obj.swigCPtr;
> }
> }
>
> // Derived proxy classes use this code
> // In my wrappers there is no reason to call back into C++ to do an upcast
> // (and SWIG doesn't support multiple inheritance anyway), so I removed that
> // code from here.
Could I ask you to add in a big DANGER sign in your comments here if
you are going to post this onto the internet again as you are relying on
behaviour that is not guaranteed in the C++ standard and is liable to
subtle, hard to track bugs, particularly so as you have suppressed
SWIGWARN_CSHARP_MULTIPLE_INHERITANCE.
> %typemap(csbody_derived) SWIGTYPE %{
> //private HandleRef swigCPtr;
>
> internal $csclassname(IntPtr cPtr, bool cMemoryOwn) : base(cPtr, cMemoryOwn) {
> // : base($imclassname.$csclassnameUpcast(cPtr), cMemoryOwn) {
> //swigCPtr = new HandleRef(this, cPtr);
> }
>
> //internal static HandleRef getCPtr($csclassname obj) {
> // return (obj == null) ? new HandleRef(null, IntPtr.Zero) : obj.swigCPtr;
> //}
> %}
>
> -------------------------------------------------------------------
>
>> Please try it out in the next day, just use the latest std_vector.i by
>> putting it in whichever version of SWIG you have. Get it from:
>> http://swig.svn.sourceforge.net/viewvc/swig/trunk/Lib/csharp/std_vector.i?
>> view=log
>
> OK, I'll try to remember to try it. I can't try it until Monday though.
>
>> Are you saying that the current enumerator implementation, which as you
>> say uses objects, can be improved performance-wise? Should we replace
>> storing as 'object' with the real collection type? It'll need a slight
>> rewrite though as casting null to primitive types (if collection type is
>> primitive) doesn't work.
>
> Yes, you'll get higher performance using the real collection type, especially if the real collection
type is something simple like int. I think it's legal to use default(xyz) instead of null, e.g.
default(int) is 0 and default(string) is null.
Okay good news, but if default(int) will give 0, it is a valid value for
int, which breaks the logic in the Current property (which is expecting
null), so a minor rewrite is needed.
The idea of overriding Equals and GetHashCode and overloading operator==
makes sense as it provides reference equals on the C++ instance level
rather than on C# instances and I'm thinking of putting this as a
default into SWIG. However, using a single base class implementation of
this is incorrect and instead I'd generate the methods into every proxy
class.
With regard to using the C++ operator==, the advice given in the MSDN
links above says: "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)". Surely, if there is a C++ operator== the advice then
applies and we should be using value equality. If say we are wrapping
std::vector<T> where T is a class (not a pointer to a class), then using
reference equals is just plain wrong as we are storing the object by
value, not reference/pointer. If we are wrapping
std::vector<T*> or std::vector<T&> then implementing reference equality
makes sense and we could modify the specializations in std_vector.i to
do this.
William
------------------------------------------------------------------------------
RSS Feed