boost::python, returning new PyObject references

[David Abrahams]

Remarks:

tmodule.cpp -

    * (Trivial) please use spaces instead of tabs - this applies to
      all of your files.
    * It doesn't test anything other than that certain constructs
      compile. There should be assertions or other semantic checks
      that verify the behavior.
    * What is the purpose of the large commented-out section at the top?
    * The reason you get a compile error from handle_exception() is that
      the interface for using it has changed in CVS (the documentation
      hasn't been updated yet). It now takes a boost::function0<void>
      parameter which represents the operation whose exceptions should
      be handled, returning true if there was an exception and false
      otherwise. This was done to account for the many compilers with
      buggy EH mechanisms that don't handle the catch-all-and-rethrow
      idiom correctly.
    * There are no tests for the generic sequence and mapping. Is that
      intentional?

objects_b.hpp -

    * It seems to me that "callback" is an insufficiently descriptive name.
      Maybe "set_function" or "assignment_action" would be better... if
that's
      what it does. What is the purpose of the callback?
    * It seems to me that the callback is misplaced in object_base, since
only
      the mapping_proxy makes any use of it.
    * Why does sequence::append call PyListe_Append? That doesn't seem like
      the generic interface for sequence we had in mind.
    * Shouldn't sequence inherit privately from object? I don't think that a
      sequence in general has a __call__ operation. What we discussed was
      a fully-generic class "object" which supplies the full Python object
      interface, with privately derived classes for more-restrictive
interfaces,
      using using-declarations to expose public access to the appropriate
      functions.
    * The lack of comments in the code makes it hard to understand what
you're
      aiming at.

objects.hpp -

    * Why are there separate sequence and mapping proxies if they both carry
a
      callback to do the work? It looks to me like a single proxy class
would be
      more appropriate.
    * Some of your lines are so long that they're difficult to understand,
e.g.:

explicit proxy(ref item, boost::function<void,ref> s_callback,
boost::function<ref> get_callback):  object(item) ,
sequence::proxy(object::reference(),s_callback).

      The following would be easier:

        explicit proxy(
            ref item
            , boost::function<void,ref> s_callback
            , boost::function<ref> get_callback)
         :  object(item)
         , sequence::proxy(object::reference(),s_callback)

      I usually use 70-80 columns as a guideline, FWIW.

    * Lots of commented-out code in this file obscures your intention also.
    * What is the point of having objects.hpp and objects_b.hpp? Are there
      advantages/disadvantages to one or the other?

----- Original Message -----
From: "Arnaldur Gylfason" <arnaldur at decode.is>
To: "David Abrahams" <david.abrahams at rcn.com>
Cc: "Xavier Defrang" <xavier at perceval.net>; "Ullrich Koethe"
<koethe at informatik.uni-hamburg.de>; "Ralf W. Grosse-Kunstleve"
<rwgk at cci.lbl.gov>; "Niklas Blomberg" <niklas_blomberg at telia.com>; "Martin
Casado" <casado2 at llnl.gov>; "Mark Evans" <evans at kandctech.com>; "Lyle
Johnson" <ljohnson at resgen.com>; "Barry Scott" <barry at scottb.demon.co.uk>;
"Anton Gluck" <gluc at midway.uchicago.edu>; "Alex Martelli"
<aleaxit at yahoo.com>; "Martin Casado" <mcasado at gmx.net>; "scott snyder"
<snyder at fnal.gov>
Sent: Wednesday, December 19, 2001 8:24 AM
Subject: Re: boost::python, returning new PyObject references


>
>
> Here is the code.
> objects.hpp and objects.cpp
>
> Next step is making final (for the moment) decision regarding design and
> the going on from there.
> Adding the operators to be able to do m["k"] += 3 etc. is probably next.
> Changing all proxies/ op[] in other classes should be done.
>
>
> objects_b.hpp has the proxies that do not use the callbacks.
> tmodule.cpp is a test module.
> (See attached file: objects.hpp)(See attached file: objects_b.hpp)(See
> attached file: objects.cpp)(See attached file: tmodule.cpp)
>
> Comments:
>
> object is a virtual base class for sequence and mapping.
> seqmap inherits from both.
> The proxies inherit from seqmap and thus provide both a sequence and
> mapping interface.
>
> If a non-existent key is accessed, a Null ref (default constructed) is
> stored in the proxies object_base.
> Then the mapping::proxy stores a callback in the object_base that will
call
> PyObject_GetItem when used in
> any way other than setting a new key-value pair.
>
> The proxies call reset after setting a new item to reflect the change.
> That means the behaviour of multiple assignment is as would be expected:
>           python::sequence seq2 = seq[2] = 5;
> I.e. seq2 gets 5 (instead of 3 which was the old value).
>
> const correctness is respected: If items are accessed for const
> sequences/mappings,
> only const methods in object, sequence or mapping can be used.
> All methods that can possibly change the underlying PyObject are
non-const.
>
> Constructors for seqmap and proxies call the object constructor directly
> because it is a virtual base class.
> There is no default constructor for the virtual base class.
> Many consider that bad practice but a default constructor wouldn't make
> sense in this case I think.
>
> Implicit Copy constructor and destructor respect virtual base classes.
> According to the version of the Draft standard I found, the operator=
could
> assign the virtual base class
> more than once - unspecified behaviour. => seqmap::operator= is defined.
>
> The copy constructor is specified in the object_base because of strange
> behaviour.
> When the ref holds a NULL object a Segmentation Fault occurs if it is used
> to copy construct another one.
> (Since the ref copy constructor calls Py_XINCREF it should handle NULL ,
> any ideas?).
> Any way, by directly passing reference() in this case the callback is
used.
> In case of illegal key access
> the Key Error is thrown here.
>
>
> I believe all concrete object interfaces (like tuple, list, dictionary,
> ...)should inherit from object_base.
> If their interface has all relevant member functions there is no need to
> inherit from object or sequence.
> The idea of list<>, list<object>, list<sequence> , ...:
>      template<class ObjectType = object_bas>
>      list : public ObjectType
> is interesting.
> When ObjectType is sequence or other class that inherits virtually, a
> problem arises.
> The constructor would have to call the object constructor directly.
> Hard if inherited fdrom a template parameter.
> For now I believe just inheriting from object_base is good.
> What do you think?
>
> Cheers
>
> Arnaldur
>