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

 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.
 - 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).
 - 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_];
 }}}
 - 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");
     }
 }}}
 - In general, I'd like to avoid using `Name` members when possible:
   Use getLabelCount() instead of `Name::labelcount_`;  Use getDataLength()
   instead of `Name::length_`.
 - 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)
 - 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;
         }
 }}}
 - 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.
 - 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.

 - 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.
 - I don't see the need for this check in the context of this test
 {{{#!cpp
     EXPECT_THROW(ls7.stripLeft(1), isc::OutOfRange);
 }}}
 - testing another boundary case, i.e., a longest possible name (label
   seq) and possible longest labels would be a good idea.
 - If we really want to allow `LabelSequence::toText()` to take the
   omit_final_dot parameter, we should have tests for that version,
   too.

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