BIND 10 #2148: extendable LabelSequence

[BIND 10 Development]

#2148: extendable LabelSequence
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jelte
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120731
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  libdns++                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jelte


Comment:

 Hi Jelte

 Here are my comments:

 * In `LabelSequence::extend()`, the `absolute` variable is not necessary:

 {{{
 -    bool absolute = isAbsolute();

 -    if (absolute) {
 +    if (isAbsolute()) {
 }}}

 If you must keep it, please make it `const`.

 * `const` can be added for `append_label_count` too.

 * The following code in `LabelSequence::extend()` can be replaced with
 `memmove()`:
 {{{
     // Note: In theory this could be done with a straightforward memcpy.
     // However, one can extend a labelsequence with itself, in which
     // case we'd be copying overlapping data (overwriting the current last
     // label if this LabelSequence is absolute). Therefore we do this
     // manually, and more importantly, backwards.
     // (note2: obviously this destroys data_len, don't use below,
     // or reset it)
     while (--data_len) {
         buf[data_pos + data_len] = data[data_len];
     }
     buf[data_pos + data_len] = data[data_len];
 }}}

 Such byte-by-byte copying is expensive on some architectures. Standard
 library is better.

 * Do we need to accept `buf` as an argument to `LabelSequence::extend()`?
 It validates that the buffer variant of a LabelSequence instance is used,
 but I think it's better to use an internal flag in this case and check it
 in `extend()` rather than expect the caller to pass `buf` each time. That
 would avoid one exceptional case too.

 * If you can, please s/added/appended/ in the `LabelSequence::extend()`
 comment:
 {{{
 +    /// The given labels are added to the name data, and internal offset
 }}}

 * In the following case, the "appended LabelSequence" is confusing as it
 can be assumed as `this` too. Better name the `labels` argument.
 {{{
 +    /// The data from the given LabelSequence is copied into the buffer
 +    /// associated with this LabelSequence; the appended LabelSequence
 +    /// can be released if it is not needed for other operations anymore.
 }}}

 * Again a matter of personal opinion :), but can we use "!=" instead of
 "<>" in `check_equal()`?

 * Can we rename `extendableLabelSequenceStrippedSource` to
 `extendableLabelSequenceLeftStrippedSource` to match the right case?

 * In `ExtendableLabelSequenceTest.extend`, the `check_compare()` call's
 args are probably not what you intended, or this is misplaced:
 {{{
 +    // Extending again should make it different
 +    els.extend(ls3, buf);
 +    check_compare(ls1, ls2,
 isc::dns::NameComparisonResult::COMMONANCESTOR, 1);
 }}}


 * `order` should be added to `check_compare()`. This is extra baggage, but
 every time we have a non-equal relation, we must check if we got back the
 expected order.

 * Typo here:
 {{{
 +    // Once more, now start out with non-absolue
 }}}

 * I suggest two changes to tests:
  * After every `extend()` call, check the `isAbsolute()` flag.
  * After every `extend()` call, check the data of the LabelSequence
 (either `check_equal()` with an existing LabelSequence or data compare)

 * Tests should be added for the following (one of these is implicitly
 checked, but it's better to have prior explicit tests earlier):
  * Copying absolute label sequence with buffer creates absolute label
 sequence
  * Copying non-absolute label sequence with buffer creates non-absolute
 label sequence
  * Extend a non-absolute label sequence with "." and check that it becomes
 absolute

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