BIND 10 #1245: pydnspp as dynamic module (so other wrappers can use it)

[BIND 10 Development]

#1245: pydnspp as dynamic module (so other wrappers can use it)
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jinmei
                       Type:  task   |                Status:  reviewing
                   Priority:  major  |             Milestone:
                  Component:         |  Sprint-20110927
  libdns++                           |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DNS
Feature Depending on Ticket:         |  Estimated Difficulty:  0
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:3 jinmei]:
 > The basic idea seems to be good.  I have some comments about details
 > (below).
 >
 > And, as usual, I made trivial changes/cleanups directly on the branch.
 > Regarding PY_SSIZE_T_CLEAN in the first of my two commits, one of the
 > tests failed strangely in my environment, so it was actually
 > necessary, not only to be "compliant".
 >

 ok, thanks.

 > '''general'''
 >
 > - I'd suggest hiding initModulePart_xxx() as much as possible because
 >   they are essentially private internal subroutines for
 >   PyInit_pydnspp() and are not expected to be called by any others
 >   (right?).  Specifically, I'd move the declaration of them from
 >   the semi-public header files such as edns_python.h to some special
 >   header files such as edns_python_internal.h with comments that they
 >   shouldn't be included in any other files outside the pydnspp module.
 >   I'd also "hide" the declaration and definition of these
 >   initModulePart functions in a deeper namespace such as
 >   isc::dns::python::internal.
 >

 I moved them to isc::dns::python::internal. I also created a separate
 _internal
 header file for each file, when I realized that this isn't actually really
 that
 useful; I opted to define them as forward declarations in pydnspp.cc. This
 way
 we save the 'pollution' of another 17 files, and they can't be included by
 accident. We *could* make a pydnspp.h that does it, but this way seems
 fine to
 me.

 > - Relating to the previous point, I guess it would be better to hide
 >   definition details of s_XXX classes.  In particular, now that they
 >   are semi-public, it seems dangerous to keep the member variable
 >   ('cppobj', 'edns', etc) public, because a buggy binding code can
 >   break the integrity of a pydnspp object.  I don't (yet) see how
 >   exactly helper function like PyName_ToName() will be used, but that
 >   seems to be the right way to go.  Ideally, we should complete hide
 >   the existence of these classes in implementation files.  If that's
 >   impossible or not very feasible, we should at least make the member
 >   variables private.
 >

 Done. Needed to add a number of other X_ToX and createXObject functions.

 > - And, related to the previous point, I wonder why PyXXX_ToXXX() has
 >   to return a mutable reference.  Returning a mutable reference this
 >   way is as dangerous as exposing the corresponding member variable as
 >   public.  If this cannot be a const, I'd suggest returning a copy of
 >   the object, not a reference.  (I understand using a copy has its own
 >   drawbacks: It's generally more expensive, and some classes are not
 >   even copyable)
 >
 > - It's not necessary incorrect, but to be consistent you might want to
 >   rename s_RRTTL::rrttl to s_RRTTL::cppobj.  Same for s_Message (and
 >   some others).
 >

 done.

 > - PyXXX_Check and PyXXX_ToXXX should probably be better named without
 >   "_" according to the coding guideline (the same comment could apply
 >   to initModulePart_xxx() and perhaps some others, but I specifically
 >   mentioned these two because they are semi-public).
 >

 as discussed on jabber, in this case i opted for following the python
 standard.
 I did not change it, but would not object too strongly, as long as we are
 internally consistent.

 > '''Makefile.am'''
 >
 > I suspect you should be able to cleanup most (or perhaps all) of
 > EXTRA_DIST now.
 >

 ohyeah, done

 > '''edns_python.h'''
 >
 > - there's no exception-related things after this part
 > {{{
 > +// Declaration of the custom exceptions
 > +// Initialization and addition of these go in the module init at the
 > +// end
 > }}}
 >   Same for rrclass_python.h.
 >

 Yeah i found some more while i was going through the code for the first
 couple
 of things here.

 > - I don't see the point of (or need for) the changes in
 >   messagerenderer_python.h.  Same for rcode_python.h.
 >

 It may be trivial, but using the declaration from the header instead of a
 forward declaration seemed more clean to me.

 > - PyName_Check, PyName_ToName: should we check and reject NULL
 >   pointers more explicitly?
 >

 Hmm. I added checks for XXX_ToXXX, but not for the conversion; in the case
 of
 conversion, is is expected that the passed object has been verified to be
 of
 the correct type. Obviously NULL is not of the correct type :)
 (it could be contested that we don't actually call check here ourselves,
 but i
 specifically skipped this check because *most* of the uses are directly
 after
 ParseTuple("O!"), in which case the type is guaranteed.

 Note that we could also choose to return false if the argument to _Check()
 is
 NULL. However, IMO it really is a programming error, and should raise
 (returning false might hide problems).

 > '''pydnspp_common.cc'''
 > - out of curiosity, for what config.h is needed?
 > {{{
 > #include <config.h>
 > }}}

 hmm, dunno where that came from. Doesn't appear to do anything, removed.

 > - I suspect order is (fortunately) not important any more:-)
 > {{{
 > // order is important here!
 > }}}

 :)

 May have noted this already, but i also removed a number of other comments
 that
 are now unnecessary/unrelated.

 > - It doesn't have to be solved in this ticket, but I guess we should
 >   eventually move these outside pydnspp:
 > {{{
 > +// For our 'general' isc::Exceptions
 > +PyObject* po_IscException;
 > +PyObject* po_InvalidParameter;
 > }}}
 >

 Yeah, I need these in 1179 as well. Haven't figured out where to put them
 yet
 though.

 > '''rrset_python.cc'''
 > - createRRsetObject is not exception safe.  I suggest using
 >   CPPPyObjectContainer just like createNameObject().  Note also that

 I initially tried that, but the template is too pointer-specific, and I
 didn't
 see a clear way to make it work with 'copyable' things like shared_ptrs.

 I moved the actual allocation to the end of the function. If you know of a
 way
 to use the container, let me know :)

 >   if you do that you don't have to catch bad_alloc explicitly in this
 >   function (as commented in .h, the assumption is that the caller
 >   should take care of any exception thrown from createXXX()).

 hmm, the bad_alloc is not needed then either way (though the null check
 now
 still is)

 > - The what() string isn't appropriate in this context.  (It's not
 >   about NULL object, but surely memory allocation failure).
 > {{{
 >     } catch (const std::bad_alloc&) {
 >         isc_throw(PyCPPWrapperException, "Unexpected NULL C++ object, "
 >                   "probably due to short memory");
 >     }
 > }}}
 >   (but see the previous bullet.  depending on the resolution to that
 >   this point may become N/A).
 >

 removed it

 > '''Other'''
 >
 > Not really about this branch, but I'd like to propose a piggyback
 > cleanup suggestion for pycppwrapper_util.h.  It would be better to
 > specify 'explicit' to the CPPPyObjectContainer constructor:
 > {{{
 >     //CPPPyObjectContainer(PYSTRUCT* obj) : PyObjectContainer(obj) {}
 >     explicit CPPPyObjectContainer(PYSTRUCT* obj) :
 PyObjectContainer(obj) {}
 > }}}
 >
 > This change will break compile, so some of the binding C++ code
 > (including wrapper_template.cc) will need to be updated, too, but the
 > change should be trivial.

 ack, done.

-- 
Ticket URL: <http://bind10.isc.org/ticket/1245#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development