BIND 10 #2432: define and implement base and libdns++ version of RRsetCollection

[BIND 10 Development]

#2432: define and implement base and libdns++ version of RRsetCollection
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  vorner
            Priority:  medium        |                       Status:
           Component:  libdns++      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130108
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => vorner


Comment:

 Hi Michal

 Replying to [comment:31 vorner]:
 > For the iterator, does boost FOREACH work with the current one? Does it
 work
 > with the lower-case one? I think it might be worth an exception to the
 style
 > guide if we got working FOREACH and possibly some more std algoritms.
 But, of
 > course, documented why the style is broken.

 I think boost foreach would require some more types of iterators, support
 code
 and tests for these that are currently not implemented. If we want this
 feature,
 let's create a new ticket and estimate it. I'm afraid this ticket has
 taken
 much more than estimated time due to varying requirements (iterator
 implementation,
 method signatures, etc).

 > For the collation, I think there could be a private `mergeRRset` which
 would
 > add a new `RRset` or merge it with the existing one. It could be used in
 the
 > initial loading only and the `addRRset` would stay the same. Then the
 result of
 > find could never change when observed from outside. But that's
 suggestion of
 > the ticket to be.

 Nod :) I was thinking the same too. We can keep `addRRset()` the same way
 and
 support merging only during the initial construction.

 > Is the explicit template instantiation needed here? Wouldn't it be more
 > readable without it?
 > {{{#!c++
 > constructHelper<const char*>(filename, origin, rrclass);
 > }}}

 Actually I thought the type when explicitly specified made the code more
 readable. :)
 But perhaps that's not the case so it's removed now. Note that it's still
 required
 in the other constructor.

 > Maybe this should be in explicit `\throw`?
 > {{{#!c++
 > /// stream. The constructor throws MasterLoaderError if there is an
 > /// error during loading.
 > }}}

 Done.

 > Is `\returns` correct doxygen keyword? I've seen `\return` in our code,
 but I
 > don't remember seeing the third form.

 I have changed it to `\return` now and also updated some other occurrences
 in the BIND 10 tree.

 > If the `collection2` would be shorter here, it'd try to dereference an
 end
 > iterator, which probably wouldn't work. May I suggest using
 `ASSERT_TRUE` here,
 > so the test doesn't crash but is aborted instead if this happens?
 > {{{#!c++
 >     while (iter != collection.end()) {
 >          EXPECT_TRUE(iter2 != collection2.end());
 >          EXPECT_EQ((*iter).toText(), (*iter2).toText());
 >          ++iter;
 >          ++iter2;
 >     }
 > }}}

 Done.

 > Is the `exists` variable needed? Can't the method be called directly
 from
 > `EXPECT_TRUE`?
 > {{{#!c++
 >     bool exists = collection.removeRRset(Name("foo.example.org"),
 >                                          rrclass, RRType::A());
 >     EXPECT_TRUE(exists);
 > }}}

 Code has been shortened. :)

 I have also made a minor change to using a prefix operator++ in one place.

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