BIND 10 #2148: extendable LabelSequence

[BIND 10 Development]

#2148: extendable LabelSequence
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  muks
  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 jelte):

 * owner:  jelte => muks


Comment:

 Replying to [comment:4 muks]:
 > 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`.
 >

 no need, an initial version used it later on, but of course it is no
 longer necessary to keep, changed.

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

 ack, done

 > * The following code in `LabelSequence::extend()` can be replaced with
 `memmove()`:
 >

 doh, of course :)

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

 actually it has three reasons,
 - it saves us from that extra member (we're keeping the class as small as
 possible, jinmei even removed data length members)
 - we can keep the internal pointers for data and offsets const (and/or
 don't need to add yet another member for non-const data_)
 - since the caller needs to create the buffer and keep it in scope anyway,
 i don't think there's any problem with this approach

 tbf, when i originally read the ticket details, i was thinking perhaps we
 should actually make a subclass MutableLabelSequence, which does the
 allocation itself and would not need any of these, but given the way that
 we plan to use this, I don't think it's necessary, and this is more
 efficient.

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

 done

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

 done

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

 sure :)

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

 sure :)

 > * 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);
 > }}}
 >

 ack, good catch

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

 ok, but it needs to be optional, or we need to rewrite the tests; in the
 case of the loop in stripRightCheck, the order can differ, in both sign
 and value. So I've added a bool check_order and int order value (with a
 default of 0 so if you say check_order=false, you don't need to provide an
 unused value)

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

 fixed

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

 done, with a few exceptions, where extend() was only used as intermediate
 steps to create a bigger labelsequence (the end result is checked though)

 > * 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:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development