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

[BIND 10 Development]

#2053: LabelSequence::toText() and operator<<
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  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 jinmei):

 Replying to [comment:13 muks]:

 > > - For `LabelSequence`, I don't think it makes sense to specify
 > >   omit_final_dot=true, because we should always want to know
 > >   whether the sequence is absolute or not and therefore we should
 > >   always want to include the final_dot when it's absolute.
 > >   I guess this version of toText() exists only fro the convenience of
 > >   `Name::toText()`.  If so, I can think of two alternatives:
 > >   - make the version of `LabelSequence::toText(bool)` private and
 > >     make it's a friend of `Name`.
 > >   - unify this version of toText() to the other one so omit_final_dot
 > >     would always be false.  Let Name::toText() adjust the returned
 > >     string depending on the value of omit_final_dot and the size of
 > >     the returned string.
 >
 > I think the former approach is better. It involves use of `friend`, but
 is nicer than the second that involves further processing. The code has
 been changed.

 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()`.

 > It was necessary when this code was in the `Name` class, and remained
 when it was copied over. It has been removed now.
 >
 > > - In general, I'd like to avoid using `Name` members when possible:
 > >   Use getLabelCount() instead of `Name::labelcount_`;  Use
 getDataLength()
 > >   instead of `Name::length_`.
 >
 > Done. :)

 Actually, I meant LabelSequence::getDataLength(), not Name::getData().
 The use of the latter will soon be impossible, which was the point of
 this comment.

 > > - Related to the previous point, this comment and the code are not
 > >   really accurate:

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

 > > - 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 examined last.  Such semantics
 overloading makes the code 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.

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