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      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Hi Jinmei

 Replying to [comment:11 jinmei]:
 > I made a few style/editorial cleanups.
 >
 > - 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.

 > - The declarations of the two `toText()`s in the header have a lot of
 >   duplicates.  I'd unify them and refer to the unified text from one
 >   of them (but note that this point may become moot depending on how we
 >   resolve the previous point).

 They have been unified.

 > - We shouldn't need a loop to locate the start point:
 > {{{#!cpp
 >     for (unsigned int i = 0; i < first_label_; i++) {
 >         count = *np++;
 >         np += count;
 >     }
 > }}}
 >   e.g., This should suffice:
 > {{{#!cpp
 >     Name::NameString::const_iterator np = name_.ndata_.begin() +
 >         name_.offsets_[first_label_];
 > }}}

 Done. :)

 > - What's the purpose of this check?  This doesn't seem to be
 >   necessary.
 > {{{#!cpp
 >     if ((first_label_ > name_.labelcount_) ||
 >         (last_label_ > name_.labelcount_) ||
 >         (first_label_ > last_label_)) {
 >         isc_throw(BadValue, "Bad first label indices were passed");
 >     }
 > }}}

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

 > - Related to the previous point, this comment and the code are not
 >   really accurate:
 > {{{#!cpp
 >     // result string: it will roughly have the same length as the wire
 format
 >     // name data.  reserve that length to minimize reallocation.
 >     std::string result;
 >     result.reserve(name_.length_);
 > }}}
 >   (Consider the case where the original name contains 127 labels but
 >   the sequence has been reduced to a single label)

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

 > - Not specific to this branch, but it seems to be better to handle this
 >   as a special case:
 > {{{#!cpp
 >     if (name_.length_ == 1) {
 >         //
 >         // Special handling for the root label.  We ignore
 omit_final_dot.
 >         //
 >         assert(name_.labelcount_ == 1 && name_.ndata_[0] == '\0');
 >         return (".");
 >     }
 > }}}
 >   I'd handle that case within the while loop like this:
 > {{{#!cpp
 >         if (count == 0) {
 >             // We've reached the "final dot".  If we've not dumped any
 >           // character, the entire label sequence is the root name.
 >           // In that case we don't omit the final dot.
 >             if (!omit_final_dot || result.empty()) {
 >                 result.push_back('.');
 >             }
 >             break;
 >         }
 > }}}

 Done. :)

 > - Due to the class integrity this should actually never happen, so it
 >   could be assert:
 > {{{#!cpp
 >             isc_throw(BadLabelType, "unknown label type in name data");
 > }}}
 >   That said, with `LabelSequence` revised to allow construction from
 >   raw data, this will become less reliable, so I'm okay with keeping
 >   it as an exception.

 I've left it as it 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.

 > - This doesn't seem to be correct:
 > {{{#!cpp
 >     EXPECT_EQ("example.com.", ls2.toText());
 >     ls2.stripRight(1);
 >     EXPECT_EQ("example", ls2.toText());
 >     ls2.stripRight(1);
 >     EXPECT_EQ("", ls2.toText());
 > }}}
 >   After the first strip, it should be "example.com" (without '.').
 >   Likewise, the result of toText() should never be an empty string.
 >   There should be a bug in the main code.

 The tests were fixed and the bug was fixed.

 > - I don't see the need for this check in the context of this test
 > {{{#!cpp
 >     EXPECT_THROW(ls7.stripLeft(1), isc::OutOfRange);
 > }}}

 This was written as an assertion when the `toText()` implementation was in
 the `Name` class so that it didn't called with bogus values. It can be
 removed, but as it's a logical sequence in that list of strip tests, I
 just left it in.

 > - testing another boundary case, i.e., a longest possible name (label
 >   seq) and possible longest labels would be a good idea.

 Such tests have been added now.

 > - If we really want to allow `LabelSequence::toText()` to take the
 >   omit_final_dot parameter, we should have tests for that version,
 >   too.

 This type of `LabelSequence::toText()` is private now. `Name` is the only
 user of it and its tests check both `true` and `false` cases on it.

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