Ticket #181: CppReview.txt

File CppReview.txt, 8.6 KB (added by stephen, 9 years ago)

Review of the C++ code for revision 2111

Line 
1Reviewing revision 2111
2
3
4Common Comments
5===
6There are a number of remarks common to a number/all of the wrapper files.  These are repeated once here.
7
8Few of the wrapper functions have Doxygen headers.
9
10(Not sure if this applies to non-method functions) As the line is less than 80 characters long, the coding guidelines suggest that the opening brace of the function should be on the same line as the function name.
11
12In the RRXxx_python.cc files, there is a typographical error in the comment associated with the s_RRXxx structure.
13
14Inline comments in the <class>_type structures should be aligned vertically.  (Also, coding guidelines say that comments at the end of lines should usually be C++ style with a "//".)
15
16The static_cast<>() construct is now recommended instead of the C-style (xxx) construct.
17
18In the XXX_richcmp functions, a common construct is:
19
20    case Py_LE:
21        c = *self->rrttl < *other->rrttl ||
22            *self->rrttl == *other->rrttl;
23        break;
24    case Py_EQ:
25        c = self->rrttl->equals(*other->rrttl);
26        break;
27
28... it seems inconsistent to use the "==" on one line and "equals()" on another.
29
30Most initModule_Xxxx functions contain code along the lines:
31
32    PyModule_AddObject(mod, "Rdata",
33                       (PyObject*) &rdata_type);
34
35... the statement is fairly short, why is it split across two lines?
36
37There are a number of "TODO" or "XXX" comments in the code.
38
39
40
41experiments/python-binding/src/lib/dns/python/libdns_python_common.h
42===
43No #ifndef...#endif sentinels protecting against multiple #includes.
44
45
46experiments/python-binding/src/lib/dns/python/libdns_python_common.cc
47===
48No #include of libdns_python_common.h?
49
50
51readDataFromSequence()
52---
53The elements are deleted from the sequence one at a time as they are read.  Wouldn't it be more efficient to read all the elements first and then delete them from the sequence in one operation with PySequence_DelSlice()?
54
55
56experiments/python-binding/src/lib/dns/python/libdns_python.cc
57===
58
59
60experiments/python-binding/src/lib/dns/python/messagerenderer_python.cc
61===
62The methods skip(), trim() etc. are presumably not needed because the buffer is available in Python and can be accessed there?  But shouldn't there be an entry for writeName(), which allows for compression of the name? (Although in that case, how does the state of the buffer get communicated between the MessageRenderer class and Python?)
63
64Need to clarify whether the two elements tp_del and tp_version_tag in the messagerenderer_type object are correct. (Comment suggests that this is not clear.)
65
66In MessageRender_destroy, self->messagerenderer is zeroed after being deleted; self->outputbuffer is not.
67
68
69experiments/python-binding/src/lib/dns/python/name_python.cc
70===
71
72
73NameComparisonResult
74---
75There is a TODO comment asking if there is also a way to just not define it. (because the class is only ever used to return the result of a name comparison?).  However, as the C++ class can be created explicitly, is there a case for saying that the Python equivalent can also be created directly? (for example, for testing purposes?).
76
77
78Name
79---
80Name_init: It took a couple of minutes to realise the the Name_init contains code for the two constructors of Name. A comment to this effect would be helpful.  (The same comment applies to other places where overloaded methods are handled.)
81
82Name_str: It might be helpful to note in the comments that Name_str is not one of the methods defined in the C++ Name class. (Same for Name_richcmp.)
83
84Name_toWire: The value 255 is hard-coded as the output buffer size.  Better to use the constant Name::MAX_WIRE.
85
86Name_toWire: Suggest that something other than "n" be used as the name of a temporary bytes object.  Conventional use of "n" is as a pure number (which is what I took it for until I read the declaration carefully).
87
88Name_compare: Unless it is a very old compiler, "new" can never return NULL when creating the NameComparisonResult object.  Should there be a check as to whether an exception has been raised prior to checking if the a value was assigned to ret->ncr?  (Same goes for creation of "Name" in Name_reverse.)
89
90Name_richcmp: For consistency with the other methods, the first argument should be named "self".
91
92Name_richcmp: "XXX: should trigger an exception".  If this is "assert()" (as defined in "assert.h"), it won't if NDEBUG is defined during compilation. Also, if activated it will also call abort() and cause the program to exit. If an exception should be raised at this point, it would be safer to explicitly throw one.
93
94Name_concatenate: I'm a bit uncomfortable that the catch is only for one of the possible exceptions that can be thrown by the "Name" constructor. (Although I agree that this is the only exception that can be logically thrown; however it does rely on the internals of the constructor itself.  Perhaps a comment to that effect?)
95
96initModulePart_Name: A comment separating the code that builds the NameComparisonResult and Name objects would make the logic clearer.
97
98initModulePart_Name: When adding the constants to the module, can't the numeric values in the calls to Py_BuildValue be accessed symbolically via a call to Name::<constant_name>?
99
100initModulePart_Name: Do the exceptions have to be added to the module before the call that adds the name_type object?  If not, it would be clearer to group the latter with the rest of the code that deals with the name_type.
101
102
103experiments/python-binding/src/lib/dns/python/rrclass_python.cc
104===
105RRClass_richcmp - see comments for Name_richcmp about use of assert(0).
106
107RRClass_xx: The functions RRClass_CH/HS/IN/NONE/ANY contain virtually identical code, most of which could be factored out into a common function.
108
109
110experiments/python-binding/src/lib/dns/python/rrtype_python.cc
111===
112RRType_richcmp - see comments for Name_richcmp about use of assert(0).
113
114RRType_xx: The functions RRType_<whatever> contain virtually identical code, most of which could be factored out into a common function.  (In fact, they are so close to the equivalent RRClass functions, that the common code is a candidate for templatisation.)
115
116
117experiments/python-binding/src/lib/dns/python/rrttl_python.cc
118===
119RRTTL_init: comment is incorrect - it is a copy of the comment in RRClass_python.cc.
120
121RRTTL_init: the error handling when processing the constructor that takes an InputBuffer is different from that in RRClass and RRType.  Here, the C++ class's constructor validates that the sequence is long enough; in the python wrappers for RRClass and RRType it is the readDataFromSequence function.  (This is not a problem, just a remark on the difference.)
122
123RRTTL_toWire: although the OutputBuffer object automatically extends, it would be more efficient to initially allocate 4 bytes for the OutputBuffer (instead of 2) to hold a 32-bit TTL.
124
125
126experiments/python-binding/src/lib/dns/python/rdata_python.cc
127===
128Most modules, when defining the s_XXX structure use a "raw" pointer to additional data.  However, rdata_python used a boost::shared_ptr in its s_Rdata structure.  Is there a reason for this?
129
130Other modules have a richcmp function.  Given that Rdata has a compare() method, why is richcmp not implemented here?
131
132Rdata_init: The call to PyArg_ParseTuple seems to have one more argument than needed by the format string.
133
134RData_init: The call to createRdata() should use the created std::string str as the third argument.  (Although as an implicit conversion is taking place, the current use of s (const char*) is fine - in which case the creation of "str" is redundant.)
135
136Rdata_toWire: the initial size of two bytes for the OutputBuffer declaration is probably too small.  I would suggest either (a) don't initialize the size - let the first use set the correct size when the type of RR is known or (b) intialize it to four bytes - enough for an A record, which should comprise a significant fraction of the RRs handled.
137
138
139experiments/python-binding/src/lib/dns/python/rrset_python.cc
140===
141Same comment about use of shared_ptr as for rdata_python.cc
142
143
144experiments/python-binding/src/lib/dns/python/question_python.cc
145===
146Question_init: comment is incorrect - it is a copy of the comment in RRClass_python.cc.
147
148Question_getXxx: inconsistent style - in an "if" statement, all other code puts the opening brace on the same line as the "if".
149
150Question_toWire: the OutputBuffer is declared with the explicit constant 255 instead of the symbolic constant Name::MAX_WIRE.
151
152
153experiments/python-binding/src/lib/dns/python/method_python.cc
154===
155MessageFlags_XX/Opcode_Xxx/Rcode_Xxx/Section_Xxx: much repeated code - see comments for RRClass and RRType about identical code.
156
157Rcode_destroy: The comment is confusing - it says "We only use consts from Rcode so don't delete self->rcode here", and is then followed two lines later by "delete self->rcode".
158