BIND 10 #2053: LabelSequence::toText() and operator<<

[BIND 10 Development]

#2053: LabelSequence::toText() and operator<<
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  muks
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120717
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  libdns++                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by muks):

 Hi Jinmei

 Replying to [comment:14 jinmei]:
 > Okay, but in that case please make it clear in comments (for the
 > private method) that we use this function only as a helper for
 > `Name::toText()`.

 Comment has been updated.

 > > We can simply remove the `reserve()` call. The reallocation is going
 to not result in data copying in a paging kernel, and wire rep is small
 enough that the amount of buffer will suffice after a small number of
 reallocs (log_2 of max size).
 >
 > Yeah, in retrospect this might be a premature optimization (and these
 > methods don't have to be super efficient anyway).  I see it still
 > there, and I'm okay with either way.  If I keep it, however, we should
 > use `getDataLength()` (see above), and I'd revise the comment
 > accordingly.

 I've left it in its updated form as-is.

 >
 > > > - This comment does not make much sense any more:
 > > > {{{#!cpp
 > > >     assert(count == 0);         // a valid name must end with a
 'dot'.
 > > > }}}
 > > >   and the trick making this check pass is not clean:
 > > > {{{#!cpp
 > > >         if (labels == 0) {
 > > >             count = 0;
 > > >             break;
 > > >         }
 > > > }}}
 > > >   We should revise it so it will be a reasonable extension as a
 result
 > > >   of the generalization.
 > >
 > > I don't follow what you mean by "is not clean". If labels is 0, we
 have reached the right end of the label sequence and need to break out.
 For other cases, the `(count == 0)` assertion is still a reasonable one to
 have. So we assign 0 to count before breaking out.
 >
 > What I meant is that the above code tweaked the semantics of 'count'.
 > Due to this code, this variable is not (always) used to indicate the
 > number of characters in the label that was examined last.  Such
 semantics
 > overloading makes the code less understandable.
 >
 > And, on looking at it more closely with the bug I pointed out, I guess
 > the more fundamental issue is that it still tries to reach the end of
 > the original name data, while in `LabelSequence` it may have been
 > right-stripped.  What we should actually do is, IMO, to limit the loop
 > to the end of the sequence, and the check should be revised whether we
 > really reached there (i.e., not unexpectedly exit from the loop).
 >
 > I'm attaching a suggested patch.  With this change we don't even have
 > to do this check at all while still avoiding the bug with right-strip.
 > The suggested patch also includes suggested changes to the
 > string::reserve() (above).
 >
 > If you're okay with that, please apply it and merge with the comment
 > on the private function.

 I follow what you mean. The code also is better now.

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