Re: [Swig-devel] shared_ptr and runtime type checker

[William S F. <ws...@fu...>]

John Lenz wrote:
> On 01/28/2008 03:33 PM, William S Fulton wrote:
> > William S Fulton wrote:
> >> John Lenz wrote:
> >>> On 01/13/2008 05:05 PM, William S Fulton wrote:
> >>>
> >>>> Problem 2) Implicit conversion to base class
> >>>> The shared_ptr template allows an implicit conversion from a
> >>> derived class to a
> >>>> base class. SWIG does not know about this relationship and needs a
> >>> way to
> >>>> convert from shared_ptr<Derived> to shared_ptr<Base> should a
> >>> method take a
> >>>> shared_ptr<Base> and a shared_ptr<Derived> is passed in. The types
> >>> table needs
> >>>> modifying to deal with this. Possibly we could use something like
> >>> the SWIGTYPE
> >>>> *DYNAMIC typemaps to provide the conversion? I'm yet to see if
> >>> %implicitconv
> >>>> will solve this, but I think it has some inefficiencies involved
> >>> with it. What
> >>>> would be great is to properly support conversion operators or 
> implicit
> >>>> conversion constructors. This could be done by adding the
> >>> conversions to the
> >>>> types table via a feature so that SWIG can perform the same
> >>> implicit conversion
> >>>> that is possible in C++. If you've got this far congrats, and
> >>> hopefully I havn't
> >>>> lost you and you know what the hell I'm talking about!!
> >>>>
> >>>> Comments from the runtime type checker experts especially 
> appreciated.
> >>>>
> >>> Well, I don't quite understand everything you are doing yet (need to
> >>> look
> >>> closer), but my first thought to solve this is to call 
> SwigType_inherit
> >>> from Modules/lang.cxx or Modules/typepass.cxx
> >>>
> >> Thanks for taking a look John. I didn't get much time to examine this
> >> in detail today, so will do tomorrow. I was however thinking of
> >> changing the %types directive to also provide the appropriate casting
> >> code, but it looks non-trivial. %types calls SwigType_inherit.
> >> Something more to look at tomorrow.
> > I've been pondering over solutions to this for a while now. What I have
> > in mind requires some changes in the casting functions to take account
> > of new memory ownership complications. This is because a simple cast on
> > the shared_ptr is not possible. Consider the 'upcast' from a Derived to
> > a Base class which calls a copy constructor to increase the ref count:
> >
> > shared_ptr<Derived> *derived = ...[ Note: swig stores ptr to shared_ptr]
> > shared_ptr<Base> *base = new shared_ptr<Base>(*derived);
> >
> > Note the new shared_ptr object being created just to make the equivalent
> > to an upcast, where with normal pointers, nothing extra is created on
> > the heap:
> >
> > Derived* derived = ...
> > Base *base = (Base *)derived;
> >
> > To cope with this, I'm thinking of changig the casting function which
> > normally looks like this:
> >
> > static void *_p_DerivedTo_p_Base(void *x) {
> >     return (void *)((Base *)  ((Derived *) x));
> > }
> >
> > to contain a flag indicating whether the casting function has created
> > new memory, so it would change to:
> >
> > static void *_p_DerivedTo_p_Base(void *x, int *newmemory) {
> >     newmemory=0;
> >     return (void *)((Base *)  ((Derived *) x));
> > }
> >
> > and the shared_ptr version would be something like:
> > static void *_p_shared_ptrTDerivedTo_p_shared_ptrTBase(void *x, int
> > *newmemory) {
> >     newmemory=SWIG_CAST_NEW_MEMORY; // a new flag
> >     return (void *) new shared_ptr<Base>(* (shared_ptr<Derived> *)x ));
> > }
> >
> > The code which calls these casting functions will also need modifying,
> > ie SWIG_TypeCast and the methods that call it. I think this is just the
> > SWIG_Python_ConvertPtrAndOwn function where I can make use of the
> > current 'int flags' and 'int *own' parameters to indicate to the calling
> > function that it needs to delete the newly created memory.
>
> I would think we should try and update all callers in all the different
> languages, rather than just python.  
Agreed, although I'm just focusing on Python for the present.

> Maybe can get a helper function.
> But oh man, this is kinda a nasty problem.  
I know, having to create smart pointers on the heap and then having to 
deal with all the associated extra memory allocations is quite ironic.

> My thought is to try and do
> something like
>
> static inline shared_ptr<T> SWIG_ConvertPtrSharedPtr(...) {
>     int newmemory;
>     shared_ptr<Base *> *ptr = SWIG_ConvertPtr(..., &newmemory);
>     if (!ptr) ...
>
>     shared_ptr<Base *> val = *ptr;
>     if (newmemory)
>       delete ptr;
>     return val;
> }
>
So you are suggesting adding a layer of abstraction from SWIG_ConvertPtr 
for the typemap code, so that there is less memory handling to deal with 
in the typemap code. That would be quite good although there is an extra 
shared_ptr reference count being incurred (a slight cost). It is 
however, more exception safe as we wouldn't need a freearg typemap so 
that is quite attractive.
> And then can use it with something like
>  shared_ptr<Base *> arg0 = SWIG_TypeCastSharedPtr(..)
>
I think that should read SWIG_ConvertPtrSharedPtr instead of 
SWIG_TypeCastSharedPtr ? BTW there is another little typo, it's 
shared_ptr<Base> not shared_ptr<Base*>.
> Then we just rely the standard C copy constructors and destructors as it
> is returned from the function, and used inside the _wrap function.
>
> Then provide a typemap to use SWIG_ConvertSharedPtr instead of the old one
> for shared_ptr<SWIGTYPE> (probably not currently possible...)
Marcelo added a lot of support for SWIGTYPE used in many ways, but there 
doesn't seem to be any SWIGTYPE support for templates. Maybe it could be 
added, but for the immediate future, I'm looking at adding shared_ptr 
support with explicit typemaps and features, rather than being done 
automatically. For Java and hopefully Python, the user just needs to 
call one macro for the whole shared_ptr wrapping to work for a 
particular class, see SWIG_SHARED_PTR and SWIG_SHARED_PTR_DERIVED in the 
li_boost_shared_ptr.i testcase.

> Problem is, this isn't general for any type of conversion, because of the
> reliance on the return type.
>
It could be made generic by making SWIG_ConvertPtrSharedPtr a template.
> But I don't see how you can make this completly general, because to be
> able to delete the memory returned from the conversion, you need to cast
> the pointer to the correct type before calling delete  (so that the
> compiler knows which destructor to call).
>
Maybe I've missed something, but I don't think this is an issue. 
SWIG_ConvertPtr is only called from typemap code. The typemap knows the 
type it is expecting and the conversion (and thus the extra memory 
allocation) will only occur if the conversion is required by the 
typemap. The typemap will know the type it wants to convert to and 
therefore be able to delete it correctly. Some examples from 
Lib/python/boost_shared_ptr.i:

  %typemap(in) SWIG_SHARED_PTR_QNAMESPACE::shared_ptr< CONST TYPE > 
(void *argp, int res = 0) {
    res = SWIG_ConvertPtr($input, &argp, 
$descriptor(SWIG_SHARED_PTR_QNAMESPACE::shared_ptr< TYPE > *), 
%convertptr_flags);
    if (!SWIG_IsOK(res)) {
      %argument_fail(res, "$type", $symname, $argnum);
    } 
    if (argp) $1 = *(%reinterpret_cast(argp, $&ltype));
  }

The conversion will only take place, and the memory will only need 
deleting if the conversion from some other type to the following type 
$descriptor(SWIG_SHARED_PTR_QNAMESPACE::shared_ptr< TYPE > *) occurs, so 
we'd just add in a:

  delete (SWIG_SHARED_PTR_QNAMESPACE::shared_ptr< TYPE > *)argp;
or in this particular case the following is equivalent:
  delete ($&ltype)argp;

if 'newmemory' is set.

I need to look closer at what happens with a deeply nested inheritance 
hierarchy, say A derived from B derived from C. When converting from C 
to A, there must be a conversion function to go from C to A directly, 
not from C to B then another conversion from B to A, as then there'd be 
memory leaks.

Incidentally, I've made it so that Java/Python never gets to hold a 
shared_ptr<>(0), it will hold just 0 instead. This also makes the memory 
handling a bit easier. Well, maybe not easier, but at least there is a 
consistent approach here and I don't see point in creating memory on the 
heap to hold 0.

> > Lastly, I'd like to modify the %types directive to contain the code that
> > should go into the casting function. This would only be used for
> > shared_ptr for now, but could potentially be used to provide any custom
> > conversion code to go from one type to another. Example of extended 
> %types:
> >
> > %types(shared_ptr<Derived> = shared_ptr<Base>) %{
> >     newmemory=SWIG_CAST_NEW_MEMORY; // a new flag
> >     return (void *) new shared_ptr<Base>(* (shared_ptr<Derived> *)x ));
> > %}
>
> As I mentioned above, I don't know if this will work because to be able to
> delete the new memory, the calling function needs to know what type is
> actually returned
> class SwigSharedPointer<T> : public shared_ptr, public
> SwigAllocateMemoryHelper {
>   ...
> };
I'll come back to SwigSharedPointer if needed, but for now it seems to 
me that the calling function does know the type it is converting to.

William