BIND 10 #2098: Define and Implement `TreeNodeRRset` class

[BIND 10 Development]

#2098: Define and Implement `TreeNodeRRset` class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120904
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  6
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by vorner):

 Hello

 There's just one substantial thing. Cppcheck complains:

 {{{
 src/lib/datasrc/memory/benchmarks/rrset_render_bench.cc:153: check_fail:
 Missing bounds check for extra iterator increment in loop.
 (warning,StlMissingComparison)
 src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail:
 Member variable 'TreeNodeRRsetTest::origin_node_' is not initialized in
 the constructor. (warning,uninitMemberVar)
 src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail:
 Member variable 'TreeNodeRRsetTest::www_node_' is not initialized in the
 constructor. (warning,uninitMemberVar)
 src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail:
 Member variable 'TreeNodeRRsetTest::wildcard_node_' is not initialized in
 the constructor. (warning,uninitMemberVar)
 src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail:
 Member variable 'TreeNodeRRsetTest::ns_rdataset_' is not initialized in
 the constructor. (warning,uninitMemberVar)
 src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail:
 Member variable 'TreeNodeRRsetTest::dname_rdataset_' is not initialized in
 the constructor. (warning,uninitMemberVar)
 src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail:
 Member variable 'TreeNodeRRsetTest::a_rdataset_' is not initialized in the
 constructor. (warning,uninitMemberVar)
 src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail:
 Member variable 'TreeNodeRRsetTest::aaaa_rdataset_' is not initialized in
 the constructor. (warning,uninitMemberVar)
 src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail:
 Member variable 'TreeNodeRRsetTest::rrsig_only_rdataset_' is not
 initialized in the constructor. (warning,uninitMemberVar)
 src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail:
 Member variable 'TreeNodeRRsetTest::wildcard_rdataset_' is not initialized
 in the constructor. (warning,uninitMemberVar)
 make: *** [cppcheck] Error 1
 }}}

 Then there's some amount of details. There are few empty functions, with
 comments they are not used (all three of them are `writeName(const
 LabelSequence&, const bool` in some context). I think it would be safer to
 throw from them, to make sure they are really not used.

 The test `TreeNodeRRsetTest::toWire` contains several scopes. Why are the
 variables like `const TreeNodeRRset rrset1(rrclass_, www_node_,
 a_rdataset_, true);` numbered?

 The `checkToWireResult` and `checkTruncationResult` are very similar
 functions. Would it be possible to merge them to one?

 The conversion of name to stored data and then back when a label sequence
 is needed seems slightly overcomplicated (name->label squence->data->label
 sequence->possibly name). But I don't have a concrete idea how to simplify
 it.
 {{{#!c++
     const LabelSequence labels(realname);
     const size_t labels_storangelen = labels.getSerializedLength();
     realname_buf_ = new uint8_t[labels_storangelen];
     labels.serialize(realname_buf_, labels_storangelen);
 }}}

 I needed to read the code of the tested function and re-read the comment
 to understand it. I think it is quite confusing just by itself:
 {{{#!c++
         // Same for the owner name (note: in practice this method would be
         // called for rrsets at different nodes, so we check that
 condition
         // first).  Note also that based on the basic assumption of the
         // ZoneTree, if the nodes are different their RR classes must be
         // different.
 }}}

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