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      |
-------------------------------------+-------------------------------------

Comment (by 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".

 '''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.

 - 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.

 - 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).

 - 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).

 '''Makefile.am'''

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

 '''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.

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

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

 '''pydnspp_common.cc'''
 - out of curiosity, for what config.h is needed?
 {{{
 #include <config.h>
 }}}
 - I suspect order is (fortunately) not important any more:-)
 {{{
 // order is important here!
 }}}
 - 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;
 }}}

 '''rrset_python.cc'''
 - createRRsetObject is not exception safe.  I suggest using
   CPPPyObjectContainer just like createNameObject().  Note also that
   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()).
 - 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).

 '''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.

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