BIND 10 #2091: use encoded name data in RBNode

[BIND 10 Development]

#2091: use encoded name data in RBNode
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120731
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:13 vorner]:

 Thanks for the review.

 >  * This comment seems to be reversed. The binary is not followed by the
 node, but it is following it (the node is first, the label second):
 > {{{
 >       ...This is encoded
 > /// as opaque binary immediately followed by the main node object.
 > }}}

 Ah, good catch.  Fixed.

 >  * In this piece of code, the getName needlessly asserts a prerequisite
 it doesn't have to care about. Also, the same assert is called twice:
 > {{{#!c++
 >     const isc::dns::Name getName() const {
 >         assert(labels_capacity_ != 0); // shouldn't be called on a NULL
 node.
 >         return
 (dns::Name(dns::LabelSequence(getLabelsData()).toText()));
 >     }
 >
 >     /// \brief Return the label sequence of the node.
 >     ///
 >     /// This method returns the label sequence corresponding to this
 node
 >     /// in the form of \c dns::LabelSequence object.  Any modification
 to
 >     /// the tree can invalidate the returned \c LabelSequence object or
 copy
 >     /// of it; in general, it's expected to be used in a very limited
 scope.
 >     dns::LabelSequence getLabels() const {
 >         assert(labels_capacity_ != 0); // shouldn't be called on a NULL
 node.
 >         return (dns::LabelSequence(getLabelsData()));
 >     }
 > }}}

 You mean by "needlessly" this shouldn't never be called on a NULL
 node, or do you mean we *won't* have to worry about it because we're
 deprecating the NULL node (in the form of node object)?  If it's the
 former, it was not so obvious to me; if it's the latter, I'm okay with
 making the change at this point.

 At the moment I only removed the assert in getName().

 >  * To the same piece of code, isn't there a constructor for name which
 could be used from binary data? It could be faster than transforming it to
 text and back.

 We could, but I believe we should eventually simply deprecate this
 method (it would require updating other existing code, so I thought it
 was better to hold until we update the `MemoryDataSrc` internal).
 I've clarified this in comments.

 >  * What is the above note this one talks about? It seems to refer to the
 flag at the doxygen about the function, but that is far away.
 > {{{#!c++
 >     // Swap flags bitfields; yes, this is ugly.  The right solution is
 to
 >     // implement the above note, then we won't have to swap the flags in
 the
 >     // first place.
 > }}}

 It meant the comment immediately above the function definition, and is
 related to #2054.  I clarified that in the comment.

 >  * What is the purpose of the enum right under that comment? Why can't
 the tmp be just uint32_t?

 Hmm, actually we can.  I found I couldn't simply use std::swap and
 then thought I needed this trick (and thought if we just used uint32_t
 it might now compile due to the const or might incorrectly override
 labels_capacity_, but it doesn't seem to be the case).  I updated the
 code by just using uint32_t.

 (BTW you meant struct instead of enum).

 >  * Could the commented-out code be removed completely?
 > {{{#!c++
 >     // bb < c, no common label
 >     //comparisonChecks(chain, -1, 1,
 NameComparisonResult::COMMONANCESTOR);
 >     comparisonChecks(chain, -1, 0, NameComparisonResult::NONE);
 > }}}

 Good catch, it's a leftover.  Removed.

-- 
Ticket URL: <https://bind10.isc.org/ticket/2091#comment:16>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development