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

[BIND 10 Development]

#2098: Define and Implement `TreeNodeRRset` class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  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 jinmei):

 Thanks for the review.

 Replying to [comment:13 vorner]:

 >  There's just one substantial thing. Cppcheck complains:

 Ah, thanks for checking.  Fixed it.  There is one other unrelated
 cppcheck regression that has been fixed in master.  I've cherry-picked
 that fix too.

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

 Okay, done (in some cases I chose assert() rather than exception).

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

 Ah, these are leftovers from an older versions where there was no
 scope.  Now removed the numbers.

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

 The difficulty is, maybe it's premature though, that I wanted to make
 checkToWireResult generic so that it could be used for `OutputBuffer`
 too (as commented).  So unifying them would introduce its own
 complexity.  But I tried to unify the two cases while keeping the
 ability to support `OutputBuffer`.  And, in fact, I added one test
 case so we actually test it with an `OutputBuffer` (which results in
 exception).

 >  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);
 >  }}}

 Yeah, I know.  On looking it further, I realized it would be much
 simpler if we just copy the given Name object.  And, it would simplify
 other parts of the code.  Originally I used a plain old array,
 expecting we might want to optimize it using a pre allocated space
 from some pool.  But, it may be premature, and serialize() or
 restoring from it is also not super cheap anyway, so I revised the
 code by copying the name object and holding the copy.

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

 Is it that the code comment is not very understandable?  If the more
 formal documentation in the header file of the isSameKind() method
 is understandable, maybe we should just refer to it and simplify the
 in-code comment (I wrote the latter first in development, and then
 thought this would require some detailed explanation and did it in the
 header).  If your concern is something else, or the header description
 is not easy to understand either, please explain what's the concern.

 Finally, I made a few additional changes (19e4b20 through 49aabed).  I
 believe the commit log and change explain them.

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