Some more review comment fixes:

- fixed and handled some missed todo items
- 'equalized' comparison functions
- removed some now unnecessary casts from those functions
- inclusion guards in header file
- improved readDataFromSequence (and updated documentation)


git-svn-id: svn://bind10.isc.org/svn/bind10/experiments/python-binding@2242 e5f2f494-b856-4b98-b285-d166d9295462

src/lib/dns/python/libdns_python.cc

@@ -23,7 +23,6 @@
 //
 // And of course care has to be taken that all identifiers be unique
 
-// 
 #define PY_SSIZE_T_CLEAN
 #include <Python.h>
 #include <structmember.h>
@@ -39,16 +38,20 @@
 
 #include "libdns_python_common.h"
 
-// order is important here! (TODO: document dependencies)
+// For our 'general' isc::Exception
+static PyObject* po_IscException;
+
+// order is important here!
 #include "messagerenderer_python.cc"
-#include "name_python.cc"
-#include "rrclass_python.cc"
-#include "rrtype_python.cc"
-#include "rrttl_python.cc"
-#include "rdata_python.cc"
-#include "rrset_python.cc"
-#include "question_python.cc"
-#include "message_python.cc"
+#include "name_python.cc"           // needs Messagerenderer
+#include "rrclass_python.cc"        // needs Messagerenderer
+#include "rrtype_python.cc"         // needs Messagerenderer
+#include "rrttl_python.cc"          // needs Messagerenderer
+#include "rdata_python.cc"          // needs Type, Class
+#include "rrset_python.cc"          // needs Rdata, RRTTL
+#include "question_python.cc"       // needs RRClass, RRType, RRTTL,
+                                    //       Name
+#include "message_python.cc"        // needs RRset, Question
 
 //
 // Definition of the module
@@ -76,6 +79,9 @@ PyInit_libdns_python(void) {
         return NULL;
     }
 
+    po_IscException = PyErr_NewException("libdns_python.IscException", NULL, NULL);
+    PyModule_AddObject(mod, "IscException", po_IscException);
+
     // for each part included above, we call its specific initializer
 
     if (!initModulePart_Name(mod)) {

src/lib/dns/python/libdns_python_common.cc

@@ -15,12 +15,13 @@
 // $Id$
 
 #include <Python.h>
+#include <libdns_python_common.h>
 
 int
 readDataFromSequence(uint8_t *data, size_t len, PyObject* sequence) {
     PyObject* el = NULL;
     for (size_t i = 0; i < len; i++) {
-        el = PySequence_GetItem(sequence, 0);
+        el = PySequence_GetItem(sequence, i);
         if (!el) {
             PyErr_SetString(PyExc_TypeError,
                 "sequence too short");
@@ -34,13 +35,13 @@ readDataFromSequence(uint8_t *data, size_t len, PyObject* sequence) {
                 return -1;
             }
             data[i] = static_cast<uint8_t>(PyLong_AsLong(el));
-            PySequence_DelItem(sequence, 0);
         } else {
             PyErr_SetString(PyExc_TypeError,
                 "not a number in fromWire sequence");
             return -1;
         }
     }
+    PySequence_DelSlice(sequence, 0, len);
     return 0;
 }
 

src/lib/dns/python/libdns_python_common.h

@@ -14,6 +14,8 @@
 
 // $Id$
 
+#ifndef __LIBDNS_PYTHON_COMMON_H
+#define __LIBDNS_PYTHON_COMMON_H 1
 
 //
 // Shared functions for python/c API
@@ -27,8 +29,10 @@
 // Data must be allocated and have at least len bytes available.
 //
 // The current implementation removes read items from the
-// head of the sequence (even if it fails it removes everything that
-// it successfully read)
+// head of the sequence, unless it fails (and returns -1), in which
+// case nothing is removed
 int readDataFromSequence(uint8_t *data, size_t len, PyObject* sequence);
 
 void addClassVariable(PyTypeObject& c, const char* name, PyObject* obj);
+
+#endif // __LIBDNS_PYTHON_COMMON_H

src/lib/dns/python/message_python.cc

@@ -106,7 +106,6 @@ static PyTypeObject messageflag_type = {
     NULL,                               // tp_descr_get
     NULL,                               // tp_descr_set
     0,                                  // tp_dictoffset
-    // TODO: Can we make this NULL? (no direct constructor)
     (initproc)MessageFlag_init,         // tp_init
     NULL,                               // tp_alloc
     PyType_GenericNew,                  // tp_new
@@ -333,7 +332,6 @@ static PyTypeObject opcode_type = {
     NULL,                               // tp_descr_get
     NULL,                               // tp_descr_set
     0,                                  // tp_dictoffset
-    // TODO: Can we make this NULL? (no direct constructor)
     (initproc)Opcode_init,              // tp_init
     NULL,                               // tp_alloc
     PyType_GenericNew,                  // tp_new
@@ -598,9 +596,7 @@ Opcode_richcmp(s_Opcode* self, s_Opcode* other, int op) {
 
     // Check for null and if the types match. If different type,
     // simply return False
-    if (!other ||
-        (static_cast<PyObject*>(self))->ob_type != (static_cast<PyObject*>(other))->ob_type
-       ) {
+    if (!other || (self->ob_type != other->ob_type)) {
         Py_RETURN_FALSE;
     }
 
@@ -741,7 +737,6 @@ static PyTypeObject rcode_type = {
     NULL,                               // tp_descr_get
     NULL,                               // tp_descr_set
     0,                                  // tp_dictoffset
-    // TODO: Can we make this NULL? (no direct constructor)
     (initproc)Rcode_init,               // tp_init
     NULL,                               // tp_alloc
     PyType_GenericNew,                  // tp_new
@@ -1050,9 +1045,7 @@ Rcode_richcmp(s_Rcode* self, s_Rcode* other, int op) {
 
     // Check for null and if the types match. If different type,
     // simply return False
-    if (!other ||
-        (static_cast<PyObject*>(self))->ob_type != (static_cast<PyObject*>(other))->ob_type
-       ) {
+    if (!other || (self->ob_type != other->ob_type)) {
         Py_RETURN_FALSE;
     }
 
@@ -1159,7 +1152,6 @@ static PyTypeObject section_type = {
     NULL,                               // tp_descr_get
     NULL,                               // tp_descr_set
     0,                                  // tp_dictoffset
-    // TODO: Can we make this NULL? (no direct constructor)
     (initproc)Section_init,             // tp_init
     NULL,                               // tp_alloc
     PyType_GenericNew,                  // tp_new
@@ -1257,9 +1249,7 @@ Section_richcmp(s_Section* self, s_Section* other, int op) {
 
     // Check for null and if the types match. If different type,
     // simply return False
-    if (!other ||
-        (static_cast<PyObject*>(self))->ob_type != (static_cast<PyObject*>(other))->ob_type
-       ) {
+    if (!other || (self->ob_type != other->ob_type)) {
         Py_RETURN_FALSE;
     }
 
@@ -1341,7 +1331,6 @@ static PyObject* Message_getSection(s_Message* self, PyObject* args);
 //static PyObject* Message_beginSection(s_Message* self, PyObject* args);
 //static PyObject* Message_endSection(s_Message* self, PyObject* args);
 
-// TODO: Question not wrapped yet
 static PyObject* Message_addQuestion(s_Message* self, PyObject* args);
 static PyObject* Message_addRRset(s_Message* self, PyObject* args);
 static PyObject* Message_clear(s_Message* self, PyObject* args);

src/lib/dns/python/name_python.cc

@@ -27,10 +27,6 @@ static PyObject* po_BadEscape;
 static PyObject* po_IncompleteName;
 static PyObject* po_InvalidBufferPosition;
 static PyObject* po_DNSMessageFORMERR;
-// This one's for our 'general' exception
-// TODO: does this need an according general class?
-// should we replace it with some more direct base exception?
-static PyObject* po_IscException;
 
 //
 // Declaration of enums
@@ -128,7 +124,6 @@ static PyTypeObject name_comparison_result_type = {
     0                                         // tp_version_tag
 };
 
-// TODO: is there also a way to just not define it?
 static int
 NameComparisonResult_init(s_NameComparisonResult* self UNUSED_PARAM,
                           PyObject* args UNUSED_PARAM)
@@ -512,31 +507,28 @@ Name_richcmp(s_Name* n1, s_Name* n2, int op) {
 
     // Check for null and if the types match. If different type,
     // simply return False
-    if (!n2 ||
-        (static_cast<PyObject*>(n1))->ob_type !=
-        (static_cast<PyObject*>(n2))->ob_type
-       ) {
+    if (!n2 || (n1->ob_type != n2->ob_type)) {
         Py_RETURN_FALSE;
     }
 
     switch (op) {
     case Py_LT:
-        c = n1->name->lthan(*n2->name);
+        c = *n1->name < *n2->name;
         break;
     case Py_LE:
-        c = n1->name->leq(*n2->name);
+        c = *n1->name <= *n2->name;
         break;
     case Py_EQ:
-        c = n1->name->equals(*n2->name);
+        c = *n1->name == *n2->name;
         break;
     case Py_NE:
-        c = n1->name->nequals(*n2->name);
+        c = *n1->name != *n2->name;
         break;
     case Py_GT:
-        c = n1->name->gthan(*n2->name);
+        c = *n1->name > *n2->name;
         break;
     case Py_GE:
-        c = n1->name->geq(*n2->name);
+        c = *n1->name >= *n2->name;
         break;
     default:
         assert(0);              // XXX: should trigger an exception
@@ -668,16 +660,11 @@ initModulePart_Name(PyObject* mod) {
     po_InvalidBufferPosition = PyErr_NewException("libdns_python.InvalidBufferPosition", NULL, NULL);
     PyModule_AddObject(mod, "InvalidBufferPosition", po_InvalidBufferPosition);
 
-    // TODO; this one is a message-specific one, move to message? 
+    // This one could have gone into the message_python.cc file, but is
+    // already needed here.
     po_DNSMessageFORMERR = PyErr_NewException("libdns_python.DNSMessageFORMERR", NULL, NULL);
-    Py_INCREF(po_DNSMessageFORMERR);
     PyModule_AddObject(mod, "DNSMessageFORMERR", po_DNSMessageFORMERR);
 
-    // TODO: this one is module-level, move to libdns_python.cc
-    po_IscException = PyErr_NewException("libdns_python.IscException", NULL, NULL);
-    Py_INCREF(po_IncompleteName);
-    PyModule_AddObject(mod, "IscException", po_IscException);
-
     PyModule_AddObject(mod, "Name",
                        reinterpret_cast<PyObject*>(&name_type));
     

src/lib/dns/python/rrclass_python.cc

@@ -262,9 +262,7 @@ RRClass_richcmp(s_RRClass* self, s_RRClass* other, int op) {
 
     // Check for null and if the types match. If different type,
     // simply return False
-    if (!other ||
-        (static_cast<PyObject*>(self))->ob_type != (static_cast<PyObject*>(other))->ob_type
-       ) {
+    if (!other || (self->ob_type != other->ob_type)) {
         Py_RETURN_FALSE;
     }
 
@@ -277,10 +275,10 @@ RRClass_richcmp(s_RRClass* self, s_RRClass* other, int op) {
             *self->rrclass == *other->rrclass;
         break;
     case Py_EQ:
-        c = self->rrclass->equals(*other->rrclass);
+        c = *self->rrclass == *other->rrclass;
         break;
     case Py_NE:
-        c = self->rrclass->nequals(*other->rrclass);
+        c = *self->rrclass != *other->rrclass;
         break;
     case Py_GT:
         c = *other->rrclass < *self->rrclass;

src/lib/dns/python/rrset_python.cc

@@ -23,22 +23,6 @@
 //
 static PyObject* po_EmptyRRset;
 
-// This one's for our 'general' exception
-// TODO: does this need an according general class?
-// should we replace it with some more direct base exception?
-
-//
-// Declaration of enums
-// Initialization and addition of these go in the module init at the
-// end
-//
-
-//
-// Declaration of class constants
-// Initialization and addition of these go in the module init at the
-// end
-//
-
 //
 // Definition of the classes
 //

src/lib/dns/python/rrttl_python.cc

@@ -255,10 +255,7 @@ RRTTL_richcmp(s_RRTTL* self, s_RRTTL* other, int op) {
 
     // Check for null and if the types match. If different type,
     // simply return False
-    if (!other ||
-        (static_cast<PyObject*>(self))->ob_type !=
-        (static_cast<PyObject*>(other))->ob_type
-       ) {
+    if (!other || (self->ob_type != other->ob_type)) {
         Py_RETURN_FALSE;
     }
 
@@ -271,10 +268,10 @@ RRTTL_richcmp(s_RRTTL* self, s_RRTTL* other, int op) {
             *self->rrttl == *other->rrttl;
         break;
     case Py_EQ:
-        c = self->rrttl->equals(*other->rrttl);
+        c = *self->rrttl == *other->rrttl;
         break;
     case Py_NE:
-        c = self->rrttl->nequals(*other->rrttl);
+        c = *self->rrttl != *other->rrttl;
         break;
     case Py_GT:
         c = *other->rrttl < *self->rrttl;

src/lib/dns/python/rrtype_python.cc

@@ -104,8 +104,6 @@ RRType_AXFR(s_RRType *self);
 static PyObject*
 RRType_ANY(s_RRType *self);
 
-//TODO: do we also want specific equals? (and perhaps not even richcmp?)
-
 // This list contains the actual set of functions we have in
 // python. Each entry has
 // 1. Python method name
@@ -320,10 +318,7 @@ RRType_richcmp(s_RRType* self, s_RRType* other, int op) {
 
     // Check for null and if the types match. If different type,
     // simply return False
-    if (!other ||
-        (static_cast<PyObject*>(self))->ob_type !=
-        (static_cast<PyObject*>(other))->ob_type
-       ) {
+    if (!other || (self->ob_type != other->ob_type)) {
         Py_RETURN_FALSE;
     }
 
@@ -336,10 +331,10 @@ RRType_richcmp(s_RRType* self, s_RRType* other, int op) {
             *self->rrtype == *other->rrtype;
         break;
     case Py_EQ:
-        c = self->rrtype->equals(*other->rrtype);
+        c = *self->rrtype == *other->rrtype;
         break;
     case Py_NE:
-        c = self->rrtype->nequals(*other->rrtype);
+        c = *self->rrtype != *other->rrtype;
         break;
     case Py_GT:
         c = *other->rrtype < *self->rrtype;