[C++-sig] Wrapping opaque pointers returned from API function s

[David Abrahams]

Gottfried Ganßauge <Gottfried.Ganssauge at haufe.de> writes:

>> Gottfried,
>>
>> This is a pretty nice implementation!
> Thanks.
>
>>
>> However, there are a few slight problems that will show up on very
>> conformant C++ implementations: firstly, pointer_converter<P>::dealloc
>> has C++ linkage, so you can't stick it in the tp_dealloc slot, which
>> has to be a function pointer with 'C' linkage.  Second, a few things
>
> Thought about that myself, but as my compiiler (msvc) didn't complain ...

It is a bit too permissive in that department.

>> you are doing with pointer_converter depend on it having an initial
>> section which is layout-compatible with PyObject.  Unfortunately, the
>> C++ standard doesn't guarantee anything about object layout for
>> classes with bases, constructors, or private members (non-POD
>> types).
>
> Didn't know that. Just for curiosity: Are there real implementations
> making a difference here?

Not to my knowledge.  It's unfortunate that the C++ standard gave
implementors so much leeway they don't even seem to use :(

>> Third, your use of the typename keyword in the declaration of the 'x'
>> member is illegal.  You can only use typename for dependent types;
>> paradoxically, template arguments themselves are not dependent.  You
>> also left out a typename on the execute function.
>
> I'll make it better the next time. promised :-)
>
>>
>> Finally, I think your DEFINE_POINTER_CONVERTER macro is likely to make
>> a Python type name containing illegal characters like '*', though I
>> guess you are passing a typedef here so it's probably not a big
>> deal.
>
> As I said: I wasn't really happy with that macro; I like your your solution
> much better.
>
>>
>> Allow me to suggest the following modification (untested):
>
> I like it - especially as it does away with my macros - , and I took
> the freedom to make it work (on MSVC that is, see below):
>
>>
>> <------------->
>> // convert_opaque_pointer --
>> //
>> // usage: convert_opaque_pointer<pointer_type>("name")
>> //
>> // registers to- and from- python conversions for a type pointer_type,
>> // and a corresponding Python type called "name".
>> //
>> namespace detail
>                      ^^^^^
> Both mscv compilers (6.0 and 7) agree that this is ambigous (probably with
> boost::python::detail !?). The complaint, alas, occurs when using
> detail::dealloc!

You must have a using directive lying around somewhere, or you put
all of this into boost::python, which I didn't expect.  Anyway, we can
pick a different name.  There is probably a dealloc in
boost::python::detail already.

>> {
>>     extern "C" inline void dealloc(PyObject* self)
>                        ^^^^^
> We're using a pointer to this function; therefore this function can't be
> expanded inline, right?

Right; the inline declaration just prevents the linker from seeing
multiple definitions.

>>     {
>>         PyObject_Del(self);
>>     }
>> }
>>
>> template <class Pointer>
>> struct convert_opaque_pointer
>>     : ::boost::python::to_python_converter<
>>           Pointer, convert_opaque_pointer<Pointer> >
>> {
>>     // This is a POD so we can use PyObject_Del on it, for example.

> I see your point, but msvc doesn't accept your upcast below; 

I didn't say I tested the code ;-)

> and anyway; who guarantees that PyObject won't be declared
> differently in the next python release?

Guido does.

> What about struct instance : public PyObject { Pointer x; }; ?

No, that's non-POD.  It has a base class.  

>>     struct instance
>>     {
>>         PyObject_HEAD
>>         Pointer x;
>>     };
>>
>>     convert_opaque_pointer(char const* name)
>>     {
>>         type_object.tp_name = name;
>                                                ^^^^^ should be
> const_cast<char *> (name), as tp_name is declared char *
>>
>>         boost::python::lvalue_from_pytype<
>>             opaque_pointer_converter, &opaque_pointer_converter::type
>                ^^^^^^^^^^^^^^^^^^^^^   ^^^^^^^^^^^^^^^^^^^^^^^^
> I take it you mean
> convert_opaque_pointer<Pointer>,
> convert_opaque_pointer<Pointer>::type_object

Right.

>>         >();
>>     }
>>
>>     static PyObject* convert(Pointer x)
>>     {
>>         instance *o = PyObject_New (instance, &type_object);
>>         o->x = x;
>>         return boost::python::detail::upcast<PyObject>(o);
>                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> MSVC6 produces a compiler error here (error C2780: 'Target *__cdecl
> boost::python::detail::upcast(Source *,int *,int *,Target *)' : expected 4
> arguments - 1 supported), but it works, when I derive instance publicly from
> PyObject.

Sorry, remove the ::detail qualification.  Should be
boost::python::upcast<PyObject>(o).

>>     }
>>
>>     static typename boost::remove_pointer<Pointer>::type&
>>     execute(instance &p_)
>>     {
>>         return *p_.x;
>>     }
>>
>>     static PyTypeObject type_object;
>> };
>>
>> template <class Pointer>
>> PyTypeObject convert_opaque_pointer<Pointer>::type_object =
>> {
>>     PyObject_HEAD_INIT(NULL)
>>     0,
>>     0,
>>     sizeof(typename convert_opaque_pointer<Pointer>::instance),
>>     0,
>>     detail::dealloc
>> };
>> <------------->
>>
>> After we settle on something, I'd like to put it in the library, or at
> I would be more than glad about that.
>
>> least in the FAQ.
>>
> Finally my edition of your implementation:

<snipped>

-- 
                       David Abrahams
   dave at boost-consulting.com * http://www.boost-consulting.com
Boost support, enhancements, training, and commercial distribution